Reject response header blocks that don’t include the :status field by sunu · Pull Request #311 · python-hyper/h2

sunu · 2016-09-12T15:08:58Z

Fix for #290

Lukasa · 2016-09-12T15:20:43Z

            # an internal event, so we can do away with this branch.
            is_trailer = False

+        is_response_header = is_response and not is_trailer


is_response should be completely determinable by the events. Do we really need the extra parameter?

Lukasa · 2016-09-12T15:24:59Z

Great start @sunu! So far this looks really good, I've left a few notes inline for you to look over.

sunu · 2016-09-13T07:31:24Z

Thanks for the review @Lukasa @alexwlchan. I've updated the PR addressing most of the comments.

Lukasa · 2016-09-13T08:28:54Z

            )
+            is_response_header = isinstance(
+                events[0], ResponseReceived
+            ) and not self.state_machine.client


Do we need this and not check? I believe that ResponseReceived can only fire when we are a client.

While we're here, I suspect we're going to need an internal-only _ResponseSent event, much like the _TrailersSent event you can see in the block above.

Lukasa · 2016-09-13T08:52:35Z

Great! I have left some more notes.

nateprewitt · 2016-09-16T15:38:10Z

-        u':status' not in seen_pseudo_header_fields
-    ):
+    seen_status_field = (b':status' in seen_pseudo_header_fields or
+                         u':status' in seen_pseudo_header_fields)


@sunu I believe that the parens on their own lines is actually a style thing for this project that @Lukasa prefers. You can see is working similarly on the conditional above.

Oops. Yeah, sorry about that. Will fix.

sunu · 2016-09-16T15:41:57Z

@Lukasa I have pushed some commits. But the test test_matching_authority_host_headers will fail for all cases where is_response_header is true, because headers doesn't contain :status.

So does it make sense to add :status to headers there? Does :authority header ever appear in a response?

Lukasa

Awesome, one small comment change and then I think we're ready to go!

Lukasa · 2016-09-16T15:38:20Z

+    This is an internal event, used to determine validation steps on
+    outgoing header blocks.
+    """
+    pass


I'd like at this point to also add a _RequestHeadersSent field. We don't have to do it as part of this PR, but can you open a new issue to track adding the new internal event? The lack of symmetry is making me sad. ;)

Lukasa · 2016-09-16T15:41:34Z

        c.initiate_connection()
        c.send_headers(1, self.example_request_headers)
-        f = frame_factory.build_headers_frame(self.example_request_headers)
+        f = frame_factory.build_headers_frame(self.example_response_headers)


Heh, nice catch! That's a good validation that the new code is working as expected!

Lukasa · 2016-09-16T15:48:41Z

Response headers may never contain :authority.

sunu · 2016-09-16T15:58:59Z

Awesome, one small comment change and then I think we're ready to go!

@Lukasa which comment change are you refering to? (I have already added a comment about ResponseHeadersSent event if that's the one you're refering to)

Also, what about the failing test? :P

sunu · 2016-09-16T16:34:30Z

@Lukasa @nateprewitt Off topic but I get emails whenever you guys comment, but I can't find the last couple of comments on the Github ui anywhere (I tried for at least 5 minutes :P) Any idea what's going on?
Sorry for being a noob lol :P

Lukasa · 2016-09-16T16:35:19Z

Heh, sorry, they're in an outdated diff. If you scroll up a little bit you'll find a dropdown saying "x commented on an outdated diff": you want the one with lots of comments in it. ;)

sunu · 2016-09-16T17:14:15Z

@Lukasa PR updated

Lukasa · 2016-09-16T17:20:07Z

-                                             hdr_validation_flags):
+    def test_matching_authority_host_headers(
+        self, validation_function, hdr_validation_flags
+    ):


Heh, sorry, I know that this isn't what you were told earlier, but the original style of this function was better. Basically, for function definitions I like having the argument names lined up, but for any other multiline sections I like having the parentheses by themselves.

Ok. fixed :)

Lukasa

One tiny comment note! We're nearly there!

Lukasa · 2016-09-16T17:50:50Z

            # Some state changes don't emit an internal event (for example,
            # sending a push promise).  We *always* emit an event for trailers,
            # so the absence of an event means this definitely isn't a trailer.
+            # Similarly, we also emit an event whenever reponse headers are


This should be response. =)

alexwlchan

Overall looks pretty good to me! Just a few minor suggestions.

alexwlchan · 2016-09-17T05:44:27Z

        )

+    # If ':status' pseudo-header is not there in a response header, reject it
+    seen_status_field = (


I wonder if it’s worth moving the evaluation of seen_status_field into a conditional block that only runs if this is a response header, i.e.,

if hdr_validation_flags.is_response_header: seen_status_header = (...) if not seen_status_field: raise ProtocolError(...)

which saves us two unnecessary set lookups on request headers.

alexwlchan · 2016-09-17T05:46:44Z

-    ]
+    hdr_validation_response_headers = list(
+        filter(lambda flags: flags.is_response_header, hdr_validation_combos)
+    )


(Opinion) I’d prefer this as a list comprehension, as I think that’s easier to see at a glance what the condition is, i.e.,

hdr_validation_response_headers = [ flags for flags in hdr_validation_combos if flags.is_response_header ]

and

hdr_validation_response_headers = [ flags for flags in hdr_validation_combos if not (flags.is_trailer or flags.is_response_header) ]

but if @Lukasa is happy with list(filter(lambda(...))), I’ll shut up about it.

I have no problem with using a list comprehension.

alexwlchan · 2016-09-17T05:50:43Z

            seen_pseudo_header_fields
        )

+    # If ':status' pseudo-header is not there in a response header, reject it


It would be nice if this comment had a reference to the exact RFC section – I know we‘ve not been super consistent on that point, but I think it’s helpful.

To save you looking it up again: RFC 7540 § 8.1.2.4.

https://tools.ietf.org/html/rfc7540#section-8.1.2.4

alexwlchan · 2016-09-17T05:52:57Z

                events[0], (_TrailersSent, TrailersReceived)
            )
+            is_response_header = isinstance(
+                events[0], (_ResponseHeadersSent, ResponseReceived)


(Trivial) Seeing these side-by-side, I wonder if we should drop Headers from the name of this event, i.e., _ResponseSent. Better symmetry, and matches what I did for _TrailerSent.

Rather than deal with this in this ticket, let's open a new one related to this issue and #314 that refactors the event names.

Added as a comment on #314.

Lukasa · 2016-09-19T08:36:37Z

@sunu I'm aware that you're probably just taking a break, but as an FYI I'd like you to address @alexwlchan's reviews before I do another review. =)

sunu · 2016-09-20T18:23:35Z

@Lukasa @alexwlchan PR updated!

alexwlchan

Okay, all looks good to me!

alexwlchan · 2016-09-20T18:23:25Z

                events[0], (_TrailersSent, TrailersReceived)
            )
+            is_response_header = isinstance(
+                events[0], (_ResponseHeadersSent, ResponseReceived)


Added as a comment on #314.

Lukasa

Excellent work! This looks great to me, I'm so delighted that you finished it up. ✨

Lukasa · 2016-09-20T18:46:02Z

Can you please merge in the latest master branch? I'd like to confirm that the tests still pass with it in place.

sunu · 2016-09-20T19:23:58Z

@Lukasa Done! :)

Reject response header blocks that don’t include the field

ece998d

Lukasa reviewed Sep 12, 2016
View reviewed changes

Get rid of the unnecessary is_response flag. And some code style fixes

8fed89c

A couple more style fixes

d343543

Lukasa reviewed Sep 13, 2016
View reviewed changes

alexwlchan changed the title ~~Reject response header blocks that don’t include the field~~ Reject response header blocks that don’t include the :status field Sep 14, 2016

sunu added 2 commits September 15, 2016 22:23

Introduce _ResponseHeadersSent event

d74d28d

Use filter to generate validation flags for testing

6fcca00

nateprewitt reviewed Sep 16, 2016

View reviewed changes

Refactoring some ugly code

bceb701

sunu force-pushed the issue-290 branch from 1074566 to bceb701 Compare September 16, 2016 15:44

Lukasa requested changes Sep 16, 2016

View reviewed changes

sunu mentioned this pull request Sep 16, 2016

Emit _RequestHeadersSent internal event #314

Closed

Refactored the host-authority matching test

371706f

Lukasa requested changes Sep 16, 2016

View reviewed changes

Fix code style

92ff0e9

Lukasa requested changes Sep 16, 2016

View reviewed changes

fix typo in comment

8cb0e39

sunu force-pushed the issue-290 branch from f82b22c to 8cb0e39 Compare September 16, 2016 19:10

alexwlchan reviewed Sep 17, 2016

View reviewed changes

sunu added 2 commits September 20, 2016 23:42

add reference to the relevant RFC section

e0edfb3

Use list comprehension instead of filter

439d5dd

alexwlchan approved these changes Sep 20, 2016

View reviewed changes

Lukasa approved these changes Sep 20, 2016

View reviewed changes

Merge branch 'master' into issue-290

efb6450

Lukasa merged commit f4b2046 into python-hyper:master Sep 20, 2016

Lukasa mentioned this pull request Sep 20, 2016

Reject response header blocks that don’t include the :status field #290

Closed

sunu deleted the issue-290 branch September 20, 2016 19:37

Conversation

sunu commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Sep 12, 2016

Uh oh!

sunu commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunu commented Sep 16, 2016

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Sep 16, 2016

Uh oh!

sunu commented Sep 16, 2016

Uh oh!

sunu commented Sep 16, 2016

Uh oh!

Lukasa commented Sep 16, 2016

Uh oh!

sunu commented Sep 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexwlchan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Sep 19, 2016

Uh oh!

sunu commented Sep 20, 2016

Uh oh!

alexwlchan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Sep 20, 2016

Uh oh!

sunu commented Sep 20, 2016

Uh oh!