Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

feat: enforce strict crypto header checks #734

Merged
merged 2 commits into from
Nov 16, 2016
Merged

feat: enforce strict crypto header checks #734

merged 2 commits into from
Nov 16, 2016

Conversation

bbangert
Copy link
Member

@bbangert bbangert commented Nov 15, 2016

Explicitly verify the crypto headers are present and match either the
01 or 04 webpush encryption drafts. This also includes a refactor of
the push schemas to remove the push_validation file.

Closes #188

Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

Looks good so far.

@pjenvey
Copy link
Member

pjenvey commented Nov 15, 2016

+1

@bbangert bbangert force-pushed the feat/issue-188 branch 2 times, most recently from b2ef3fc to 3845d5a Compare November 15, 2016 22:11
@bbangert bbangert changed the title WIP feat: enforce strict crypto header checks Nov 15, 2016
@bbangert bbangert force-pushed the feat/issue-188 branch 4 times, most recently from c9ada09 to adb4562 Compare November 16, 2016 00:18
@codecov-io
Copy link

codecov-io commented Nov 16, 2016

Current coverage is 100% (diff: 100%)

Merging #734 into master will not change coverage

@@           master   #734   diff @@
====================================
  Files          47     46     -1   
  Lines        9065   9156    +91   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         9065   9156    +91   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update b2431b4...3d42b5a

Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

R+

required=False,
load_from="encryption-key",
missing=None,
validate=lambda x: x is None,
Copy link
Member

Choose a reason for hiding this comment

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

What kind of validation error message does this produce (if an encryption-key's included)? If it's not very clear we might want to raise a specific InvalidRequest with a more helpful message.

Here's marshmallow's example for rejecting specific fields:

http://marshmallow.readthedocs.io/en/latest/extending.html#validating-original-input-data

with assert_raises(InvalidRequest) as cm:
schema.load(data)

eq_(cm.exception.message, "Unknown Content-Encoding")
Copy link
Member

Choose a reason for hiding this comment

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

might as well check for the expected errno and maybe status_code here additionally

Explicitly verify the crypto headers are present and match either the
01 or 04 webpush encryption drafts. This also includes a refactor of
the push schemas to remove the push_validation file.

Closes #188
errno=110)

@validates_schema(pass_original=True)
def check_unknown_fields(self, data, original_data):
Copy link
Member

Choose a reason for hiding this comment

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

ah one more thing, let's clarify this method name. 'reject_encryption_key' would work

Explicitly verify the crypto headers are present and match either the
01 or 04 webpush encryption drafts. This also includes a refactor of
the push schemas to remove the push_validation file.

Closes #188
pjenvey pushed a commit that referenced this pull request Nov 16, 2016
* feat: enforce strict crypto header checks

Explicitly verify the crypto headers are present and match either the
01 or 04 webpush encryption drafts. This also includes a refactor of
the push schemas to remove the push_validation file.

Closes #188
@pjenvey pjenvey merged commit b4749d1 into master Nov 16, 2016
@bbangert bbangert deleted the feat/issue-188 branch November 16, 2016 22:31
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.

4 participants