Forbid pseudo-header fields not defined in RFC 7540 #193

Activity

1. Lukasa

   added

   Enhancement

   Easy

   Backport to 2.1.X

   Backport to 2.2.X

   on Apr 5, 2016
2. Lukasa commented on Apr 13, 2016

   

   Lukasa

   on Apr 13, 2016

   MemberAuthor

   More actions

   For anyone who picks this up, here are my thoughts about how you might want to approach this:

   • The easiest place to fit this in to the code is the current header validation pipeline. This lives in h2/utilities.py, and is currently composed of a pipeline of generators as shown in validate_headers in that file. In particular, a small extension to _reject_pseudo_header_fields is probably the right place to start looking to do the change.
   • For filtering things out, consider using an approach similar to the CONNECTION_HEADERS approach in that same file: a set() of whitelisted values.
   • We'll need to write tests for this. A sensible approach might be to use Hypothesis to generate those tests, but that may also be tricky for you, so if you don't want to bite that off right now just write some standard examples-based tests and put them in test_invalid_headers.TestFilter. I can go back and extend them later.
   • You don't need to do any backporting, I'll handle that.

   If you have any further questions, please do ask them.
3. LuRsT commented on Apr 13, 2016

   

   LuRsT

   on Apr 13, 2016

   Contributor

   More actions

   @Lukasa Could you take a look at my solution? I'm happy to evolve it into something better using the Hypothesis package, and I don't understand what you mean by backporting, but would be happy to know and help out more 😄
4. Lukasa commented on Apr 14, 2016

   

   Lukasa

   on Apr 14, 2016

   MemberAuthor

   More actions

   @LuRsT That change looks perfect. One small note: I'm about to merge a PR (#198) that changes our internal headers representation to be bytestrings. That'll necessitate a small update in your code: you'll need to use bytestrings in that set.

   Otherwise, that looks great: with some tests I'd be happy to merge it. =)
5. LuRsT commented on Apr 14, 2016

   

   LuRsT

   on Apr 14, 2016

   Contributor

   More actions

   Awesome @Lukasa, I'll learn about hypothesis this evening so I can use it for the tests. I'll also update my code to use bytestrings.

   One thing that I wasn't sure is the _ALLOWED_PSEUDO_HEADER_FIELDS, I don't know if I captured all the pseudo-header fields from the spec, I was expecting to find a list of them but I couldn't :/
6. Lukasa commented on Apr 14, 2016

   

   Lukasa

   on Apr 14, 2016

   MemberAuthor

   More actions

   Yeah, there is no single list, but sections 8.1.2.3 and 8.1.2.4 cover the full set. You got them. =)

   👍React with 👍1Reacted by Gil Gonçalves
7. Lukasa

   closed this as completedin 06bf6cbon Apr 14, 2016
8. Lukasa

   added 3 commits that reference this issue on Apr 25, 2016

   Disallow custom pseudo-headers

   ...

   This issue will close once commit 0afd92e is merged into the 'master' branch.

   0afd92e

   Disallow custom pseudo-headers

   ...

   This issue will close once commit d43868e is merged into the 'master' branch.

   d43868e

   Disallow custom pseudo-headers

   ...

   This issue will close once commit c254bf6 is merged into the 'master' branch.

   c254bf6