Fix 2 issues #3
Fix 2 issues #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two issues related to HTTP session handling and body size management in the pcap2har tool. The changes add support for HTTP/1.1 keep-alive connections and implement response body truncation for large payloads.
Key Changes:
- Added HTTP/1.1 keep-alive connection tracking by introducing sequence counters to differentiate multiple requests on the same TCP stream
- Implemented configurable maximum body size limit (10MB default) with truncation and warning messages for oversized responses
- Added logic to skip response body checks for HTTP HEAD requests
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pcap2har/main.py | Added HTTP/1.1 session sequencing, max body size parameter throughout the pipeline, HEAD request handling, and body truncation logic |
| tests/test_main.py | Added test for body truncation functionality |
| tests/test_http.py | Added test for HTTP keep-alive connections |
| tests/goldens/test_websocket.py-test_websocket_segmented | Updated connection ID format from "1-1" to "1-1-0" |
| tests/goldens/test_websocket.py-test_websocket_parse | Updated connection ID format from "1-0" to "1-0-0" |
| tests/goldens/test_http.py-test_chunked_gzip | Updated connection ID format from "1-0" to "1-0-0" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| if conv_details[current_session_id].request.url: | ||
| # This is a new request on the same connection, increment sequence | ||
| http1_sequence_counters[tcp_stream] += 1 |
There was a problem hiding this comment.
The session ID is created before checking if it's a new request, but the counter is only incremented after the check. This creates a race condition where current_session_id uses the old counter value but should use the new one. The counter should be incremented before creating current_session_id, or the session ID should be recreated after incrementing.
| http1_sequence_counters[tcp_stream] += 1 | |
| http1_sequence_counters[tcp_stream] += 1 | |
| current_session_id = ( | |
| "1", | |
| tcp_stream, | |
| http1_sequence_counters[tcp_stream], | |
| ) |
pcap2har/main.py
Outdated
| current_session_id = ( | ||
| "1", | ||
| tcp_stream, | ||
| http1_sequence_counters[tcp_stream], | ||
| ) | ||
| if conv_details[current_session_id].request.url: | ||
| # This is a new request on the same connection, increment sequence |
There was a problem hiding this comment.
When a request line is detected and the counter is incremented (line 364), full_stream_id on line 366 uses the incremented counter, but current_session_id still holds the old value. This means the conditional logic accesses one session while the actual processing uses a different session, leading to incorrect session assignment.
| current_session_id = ( | |
| "1", | |
| tcp_stream, | |
| http1_sequence_counters[tcp_stream], | |
| ) | |
| if conv_details[current_session_id].request.url: | |
| # This is a new request on the same connection, increment sequence | |
| # This is a new request on the same connection, increment sequence if needed | |
| if conv_details[("1", tcp_stream, http1_sequence_counters[tcp_stream])].request.url: |
| if conv.request.method != "HEAD": | ||
| content_length = conv.response.headers.get("content-length") | ||
| if content_length and int(content_length[0]) > 0 and not conv.response.body: | ||
| log_fn(f"{conv!s}: Missing response body") |
There was a problem hiding this comment.
[nitpick] The indentation change moves the response body check inside the HEAD method guard, but the request body check above (lines 296-298) is not similarly guarded. Consider documenting why HEAD responses specifically need this special handling, or extract this logic into a helper function for clarity.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| full_stream_id = ("1", tcp_stream, http1_sequence_counters[tcp_stream]) |
There was a problem hiding this comment.
The variable current_session_id is assigned but never used. This appears to be dead code that should either be removed or the logic should use current_session_id instead of full_stream_id on line 368 when processing request packets.
| full_stream_id = ("1", tcp_stream, http1_sequence_counters[tcp_stream]) | |
| full_stream_id = current_session_id | |
| else: | |
| full_stream_id = ("1", tcp_stream, http1_sequence_counters[tcp_stream]) |
Fixes #1
Fixes #2