Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2114703002:
Ignore Crypto-Key header values that do not have "dh" values (Closed)
|
Created:
4 years, 5 months ago by Peter Beverloo Modified:
4 years, 5 months ago Reviewers:
johnme CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIgnore Crypto-Key header values that do not have "dh" values
This aligns Chrome with the latest change to the webpush-encryption
specification: https://github.com/webpush-wg/webpush-encryption/pull/7
BUG=
Committed: https://crrev.com/2e8eb55d3b07742255df1fa4b3951d0d4c33c49a
Cr-Commit-Position: refs/heads/master@{#403264}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Ignore Crypto-Key header values that do not have "dh" values #
Total comments: 4
Patch Set 3 : lowercase v #
Messages
Total messages: 22 (8 generated)
peter@chromium.org changed reviewers: + johnme@chromium.org
John, PTAL
https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/gcm_encryption_provider.cc (right): https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/gcm_encryption_provider.cc:155: while (crypto_key_header_iterator.dh().empty() && The spec requires "at most one entry having a `dh` parameter" but we don't enforce that. Should we? https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right): https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:237: valid_message.data["encryption"] = kInvalidEncryptionHeader; Shouldn't this be kValidEncryptionHeader? Ditto in VerifiesCryptoKeyHeaderParsing, where you presumably copied this from?
lgtm with the nits above
https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/gcm_encryption_provider.cc (right): https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/gcm_encryption_provider.cc:155: while (crypto_key_header_iterator.dh().empty() && On 2016/06/30 16:55:58, johnme wrote: > The spec requires "at most one entry having a `dh` parameter" but we don't > enforce that. Should we? Done. https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right): https://codereview.chromium.org/2114703002/diff/1/components/gcm_driver/crypt... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:237: valid_message.data["encryption"] = kInvalidEncryptionHeader; On 2016/06/30 16:55:58, johnme wrote: > Shouldn't this be kValidEncryptionHeader? Ditto in > VerifiesCryptoKeyHeaderParsing, where you presumably copied this from? Done.
lgtm https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right): https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:48: const char kInValidThreeValueCryptoKeyHeader[] = Micro-nit: lowercase V for consistency https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:49: "keyid=foo,dh=BL_UGhfudEkXMUd4U4-D4nP5KHxKjQHsW6j88ybbehXM7fqi1OMFefDUEi0eJ" Would it be clearer to use `keyid=foo;dh` instead of `keyid=foo,dh` (similarly for baz)? Seems that would make it more like the other examples.
https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right): https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:48: const char kInValidThreeValueCryptoKeyHeader[] = On 2016/06/30 18:07:26, johnme wrote: > Micro-nit: lowercase V for consistency Done. https://codereview.chromium.org/2114703002/diff/20001/components/gcm_driver/c... components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:49: "keyid=foo,dh=BL_UGhfudEkXMUd4U4-D4nP5KHxKjQHsW6j88ybbehXM7fqi1OMFefDUEi0eJ" On 2016/06/30 18:07:26, johnme wrote: > Would it be clearer to use `keyid=foo;dh` instead of `keyid=foo,dh` (similarly > for baz)? Seems that would make it more like the other examples. This way it skips headers, which is the new logic introduced by this CL.
The CQ bit was checked by peter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2114703002/#ps40001 (title: "lowercase v")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Ignore Crypto-Key header values that do not have "dh" values This aligns Chrome with the latest change to the webpush-encryption specification: https://github.com/webpush-wg/webpush-encryption/pull/7 BUG= ========== to ========== Ignore Crypto-Key header values that do not have "dh" values This aligns Chrome with the latest change to the webpush-encryption specification: https://github.com/webpush-wg/webpush-encryption/pull/7 BUG= Committed: https://crrev.com/2e8eb55d3b07742255df1fa4b3951d0d4c33c49a Cr-Commit-Position: refs/heads/master@{#403264} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2e8eb55d3b07742255df1fa4b3951d0d4c33c49a Cr-Commit-Position: refs/heads/master@{#403264} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
