[nateprewitt]

So, I'm quickly realizing that I'm way out of my element here with HTTP2, so please excuse my naivety.

From the quick reading I did, I think this workflow should fail with a ProtocolError, but currently doesn't. I'm not sure how state works with the connection object here, so it's possible I'm not using the frames correctly.

@Lukasa or @alexwlchan, could I get some confirmation that this is along the right path?

[Lukasa]

Hrm, right now I can't see what is going wrong because I'm flying and have no access to a proper shell (I'm on a phone in an airport), and Travis is failing the build on the linter. I'll follow this up tomorrow.

[nateprewitt]

Sorry, I was expecting the test to fail so I didn't check the reason. It's now failing by NOT raising the ProtocolError. I don't know if this is because the conditions I've created are insufficient to reach that code, or because the functionality isn't implemented yet. No rush here, have a good flight!

[alexwlchan]

I don't know if this is because the conditions I've created are insufficient to reach that code, or because the functionality isn't implemented yet.

I had a proper look, and it’s because the functionality isn’t implemented yet. So you’ll need to add it – if you look in utilities.py, there are a series of functions that do header block validation. You’ll need to find an appropriate place to perform this new check.

Looking at the test already written: looks fine to me (but I’m far from an HTTP/2 expert), but you should also check that we send the appropriate response (with an HTTP error code) back down the connection. If you look at test_headers_event, you’ll see we do this check after ensuring we hit the ProtocolError.

[Lukasa]

There's no reason for us to hoist these out, they can go straight into the relevant frame factory calls.

[nateprewitt]

Thanks for clarifying that @alexwlchan!

@Lukasa, sorry about all the junk, I didn't intend to have this reviewed yet. I just wanted to clarify that the test was working as intended before extending.

I'll start working on getting that exception raised :)

[nateprewitt]

@Lukasa, it looks like this test in test_complex_logic.py may need some tweaks. It is currently sending two sets of headers, both with a :status, down a stream. When it sends the second set of headers to tear down the inbound stream, the frame is flagged as being a trailer which causes the validate_header check to fail. This doesn't fail for the what looks to be a very similar call on the outbound stream above. I'm not sure I understand why one is failing while the other isn't, other than different checks depending on "direction" of the stream.

Some possible solutions are sending a different header set (sans pseudo-headers), or just a data frame. This may need some more investigating if I'm missing something deeper though.

edit: I realized that the outbound send is for a promise frame rather than a header frame. I'm not sure what the semantics of the promise frame are, but that's probably the reason why we're not getting an error on the outbound.

[nateprewitt]

Are there different rules from inbound v.s. outbound header frames? validate_outbound_headers seems to only do a subset of validate_headers.

[Lukasa]

A PUSH_PROMISE frame contains request headers, not response headers. Essentially, the server pushes a request and then sends its own response to it.

[Lukasa]

Are there different rules from inbound v.s. outbound header frames?

No, there aren't. However, some of the checks on inbound header frames don't apply to outbound headers because we normalize them first. There's no point checking for whitespace or uppercase header fields because we got rid of any violations already.

[nateprewitt]

Ok cool, thanks @Lukasa.

Is that second header frame supposed to be treated as a new response, or a trailer? I'll need to tweak it, or my validation check to get tests passing.

[Lukasa]

There is only one HEADERS frame. =) It is, therefore, not a trailer.

[nateprewitt]

Gah, sorry I'm being so sloppy with my links. This is the second HeaderFrame we're sending down the same stream. This is being flagged as a trailer presently because my link in the comment above is the first set of headers sent.

[nateprewitt]

This fixes the failure condition but raises a question of if we really want to use hdr_validation_combos with this test. This will never be a valid trailer header and we're now checking for trailers with pseudo headers elsewhere, so this seems a bit redundant.

If this is a special case that requires all the checks though, I like this over a blanket try/except ProtocolError catch since this should be the only condition we fail under.

[Lukasa]

I think you just want to add a new set of header validation combos that doesn't include the trailer set, and use that here.

[nateprewitt]

Ok, after some further reading, I'm fairly confident the build_headers_frame issue I poorly explained earlier is in fact an unintended frame sequence. I've updated the code to send a data frame instead. Tests are passing now, and I think this should cover #263.

[Lukasa]

Ok, seeing this change in place I now understand what you were getting at. Yes, there is nothing wrong with closing that stream using trailers, but the trailers that were being sent were not actually valid trailers. This change is better. =)

[nateprewitt]

Alright, I think things are tidied. Gonna call it a night but I'll finish off any final tweaks in the morning.

[Lukasa]

Eh, let's just show them all. We can just show the set directly. =)

[nateprewitt]

Voilà!

[Lukasa]

Alright, fantastic, thanks so much @nateprewitt! Per our one of the team policy, you've got an invite to the Python Hyper organisation in your inbox. Welcome on board!