From 25fdea2983db0162a58064c5b47ba3ec24856d29 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Mon, 14 Apr 2025 09:30:59 +0100 Subject: [PATCH 1/7] fix: convert buffer where frame is stored to binary encoding before packing and prepending the frame header certain DATA payload inputs were keeping the original encoding right before prepending the binary header, which could generate an incompatible encoding error Fixes https://github.com/HoneyryderChuck/httpx/issues/83 --- lib/http/2/framer.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/http/2/framer.rb b/lib/http/2/framer.rb index 7ed228f..b8242e1 100644 --- a/lib/http/2/framer.rb +++ b/lib/http/2/framer.rb @@ -148,9 +148,12 @@ def common_header(frame, buffer:) header = buffer + # make sure the buffer is binary and unfrozen if buffer.frozen? header = String.new("", encoding: Encoding::BINARY, capacity: buffer.bytesize + 9) # header length - header << buffer + append_str(header, buffer) + else + header.force_encoding(Encoding::BINARY) end pack([ @@ -342,6 +345,13 @@ def generate(frame) raise CompressionError, "Invalid padding #{padlen}" end + # make sure the buffer is binary and unfrozen + if bytes.frozen? + bytes = bytes.b + else + bytes.force_encoding(Encoding::BINARY) + end + length += padlen pack([padlen -= 1], UINT8, buffer: bytes, offset: 0) frame[:flags] += [:padded] From 028b14df5e8368709886bb934110b65b93d3bf08 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Mon, 14 Apr 2025 09:32:40 +0100 Subject: [PATCH 2/7] copy PING payload before encoding lib users will store the ping data input to compare when receiving the peer PING frame, so it may be surprising if the data does not match because the mutable arg was changed --- lib/http/2/framer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/http/2/framer.rb b/lib/http/2/framer.rb index b8242e1..2a36425 100644 --- a/lib/http/2/framer.rb +++ b/lib/http/2/framer.rb @@ -271,7 +271,7 @@ def generate(frame) append_str(bytes, frame[:payload]) when :ping - bytes = frame[:payload] + bytes = frame[:payload].b raise CompressionError, "Invalid payload size (#{bytes.size} != 8 bytes)" if bytes.bytesize != 8 length = 8 From 4df0db78528efcd1ebdcea37d1694963c4815386 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Mon, 14 Apr 2025 09:33:11 +0100 Subject: [PATCH 3/7] small opt: check hash key instead of performing full lookup --- lib/http/2/connection.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/http/2/connection.rb b/lib/http/2/connection.rb index ce247a0..201ce87 100644 --- a/lib/http/2/connection.rb +++ b/lib/http/2/connection.rb @@ -403,8 +403,9 @@ def receive(data) # endpoints MAY choose to treat frames that arrive a significant # time after sending END_STREAM as a connection error. when :rst_stream - stream = @streams_recently_closed[stream_id] - connection_error(:protocol_error, msg: "sent window update on idle stream") unless stream + unless @streams_recently_closed.key?(stream_id) + connection_error(:protocol_error, msg: "sent window update on idle stream") + end else # An endpoint that receives an unexpected stream identifier # MUST respond with a connection error of type PROTOCOL_ERROR. From bbb7d688f0e4798ba2e17e1fbfc1e174a63045bd Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Mon, 14 Apr 2025 11:27:54 +0100 Subject: [PATCH 4/7] stream: remove assignment to @state in #manage_state calls to #event already transition @state to the next state, so this is redundant --- lib/http/2/stream.rb | 52 ++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/lib/http/2/stream.rb b/lib/http/2/stream.rb index b72ed3d..bd9fbe9 100644 --- a/lib/http/2/stream.rb +++ b/lib/http/2/stream.rb @@ -405,19 +405,19 @@ def transition(frame, sending) # WINDOW_UPDATE on a stream in this state MUST be treated as a # connection error (Section 5.4.1) of type PROTOCOL_ERROR. when :reserved_local - @state = if sending - case frame[:type] - when :headers then event(:half_closed_remote) - when :rst_stream then event(:local_rst) - else stream_error - end - else - case frame[:type] - when :rst_stream then event(:remote_rst) - when :priority, :window_update then @state - else stream_error - end - end + if sending + case frame[:type] + when :headers then event(:half_closed_remote) + when :rst_stream then event(:local_rst) + else stream_error + end + else + case frame[:type] + when :rst_stream then event(:remote_rst) + when :priority, :window_update + else stream_error + end + end # A stream in the "reserved (remote)" state has been reserved by a # remote peer. @@ -434,19 +434,19 @@ def transition(frame, sending) # PRIORITY on a stream in this state MUST be treated as a connection # error (Section 5.4.1) of type PROTOCOL_ERROR. when :reserved_remote - @state = if sending - case frame[:type] - when :rst_stream then event(:local_rst) - when :priority, :window_update then @state - else stream_error - end - else - case frame[:type] - when :headers then event(:half_closed_local) - when :rst_stream then event(:remote_rst) - else stream_error - end - end + if sending + case frame[:type] + when :rst_stream then event(:local_rst) + when :priority, :window_update + else stream_error + end + else + case frame[:type] + when :headers then event(:half_closed_local) + when :rst_stream then event(:remote_rst) + else stream_error + end + end # A stream in the "open" state may be used by both peers to send # frames of any type. In this state, sending peers observe From ded4d10809d25fdcf776b4e2a04537d79cd9069f Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Mon, 14 Apr 2025 11:30:22 +0100 Subject: [PATCH 5/7] improve priority stream management remove all calls to "#process_priority" inside "#transition", as it should only handle state machine transitions. besides, #receive already does that type of frame processing calls before calling "#transition" . this also corrects stream state management when it comes to taking priority frames into account when sending, as per the documentation on each state block, which explicitly states the cases which were not being handled. --- .rubocop_todo.yml | 2 +- lib/http/2/stream.rb | 49 +++++++++++++++++--------------------------- spec/stream_spec.rb | 4 ++-- 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e1f9558..4f416c3 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -31,7 +31,7 @@ Metrics/CyclomaticComplexity: # Offense count: 29 # Configuration parameters: CountComments. Metrics/MethodLength: - Max: 134 + Enabled: false # Offense count: 1 # Configuration parameters: CountKeywordArgs. diff --git a/lib/http/2/stream.rb b/lib/http/2/stream.rb index bd9fbe9..796f3b5 100644 --- a/lib/http/2/stream.rb +++ b/lib/http/2/stream.rb @@ -197,21 +197,18 @@ def calculate_content_length(data_length) # # @param frame [Hash] def send(frame) - process_priority(frame) if frame[:type] == :priority - case frame[:type] when :data - # @remote_window is maintained in send_data - send_data(frame) + # stream state management is maintained in send_data + return send_data(frame) when :window_update - manage_state(frame) do - @local_window += frame[:increment] - emit(:frame, frame) - end - else - manage_state(frame) do - emit(:frame, frame) - end + @local_window += frame[:increment] + when :priority + process_priority(frame) + end + + manage_state(frame) do + emit(:frame, frame) end end @@ -384,7 +381,7 @@ def transition(frame, sending) else event(:open) end - when :priority then process_priority(frame) + when :priority else stream_error(:protocol_error) end end @@ -409,6 +406,7 @@ def transition(frame, sending) case frame[:type] when :headers then event(:half_closed_remote) when :rst_stream then event(:local_rst) + when :priority else stream_error end else @@ -444,6 +442,7 @@ def transition(frame, sending) case frame[:type] when :headers then event(:half_closed_local) when :rst_stream then event(:remote_rst) + when :priority else stream_error end end @@ -465,12 +464,14 @@ def transition(frame, sending) when :data, :headers, :continuation event(:half_closed_local) if end_stream?(frame) when :rst_stream then event(:local_rst) + when :priority end else case frame[:type] when :data, :headers, :continuation event(:half_closed_remote) if end_stream?(frame) when :rst_stream then event(:remote_rst) + when :priority end end @@ -490,10 +491,7 @@ def transition(frame, sending) case frame[:type] when :rst_stream event(:local_rst) - when :priority - process_priority(frame) - when :window_update - # nop here + when :priority, :window_update else stream_error end @@ -502,10 +500,7 @@ def transition(frame, sending) when :data, :headers, :continuation event(:remote_closed) if end_stream?(frame) when :rst_stream then event(:remote_rst) - when :priority - process_priority(frame) - when :window_update - # nop here + when :priority, :window_update end end @@ -534,9 +529,7 @@ def transition(frame, sending) else case frame[:type] when :rst_stream then event(:remote_rst) - when :priority - process_priority(frame) - when :window_update + when :priority, :window_update # nop else stream_error(:stream_closed) @@ -584,19 +577,15 @@ def transition(frame, sending) when :closed if sending case frame[:type] - when :rst_stream # ignore - when :priority - process_priority(frame) + when :rst_stream, :priority else stream_error(:stream_closed) unless frame[:type] == :rst_stream end - elsif frame[:type] == :priority - process_priority(frame) else case @closed when :remote_rst, :remote_closed case frame[:type] - when :rst_stream, :window_update # nop here + when :priority, :rst_stream, :window_update # nop here else stream_error(:stream_closed) end diff --git a/spec/stream_spec.rb b/spec/stream_spec.rb index fd9afb1..c30d237 100644 --- a/spec/stream_spec.rb +++ b/spec/stream_spec.rb @@ -57,7 +57,7 @@ end it "should raise error if sending invalid frames" do - frame_types.reject { |frame| %i[headers rst_stream].include?(frame[:type]) }.each do |type| + frame_types.reject { |frame| %i[headers rst_stream priority].include?(frame[:type]) }.each do |type| expect { stream.dup.send type }.to raise_error InternalError end end @@ -114,7 +114,7 @@ end it "should raise error on receipt of invalid frames" do - frame_types.reject { |frame| %i[headers rst_stream].include?(frame[:type]) }.each do |type| + frame_types.reject { |frame| %i[headers rst_stream priority].include?(frame[:type]) }.each do |type| expect { stream.dup.receive type }.to raise_error InternalError end end From f79246409bc5bfdeea6dd07565f65ab43ea75172 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Tue, 15 Apr 2025 00:31:07 +0100 Subject: [PATCH 6/7] bump version to 1.1.1 --- CHANGELOG.md | 12 ++++++++++++ lib/http/2/version.rb | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e04276..761e16d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,15 @@ +## 1.1.1 + +### Bugfixes + +* frame buffer was accidentally changing encoding before header packing, which raise invalid compatible encoding errors, due to usage of "String#". this was fixed by using internal `append_str´, which does not touch encoding, and calling `String.force_encoding` in case the buffer is a mutable string passed by the user. +* dup PING frame payload passed by the user; while not really resulting in invalid encoding, the change of the input string could surprise the caller, since this would be expected to be stored somewhere so the peer PING frame can be matched on receive. + + +### Improvements + +Simplified `String#transition`, making sure it only does state machine transitions (the rest is handled outside of it). + ## 1.1.0 Several changes which improved performance for the common cases. A few highlights: diff --git a/lib/http/2/version.rb b/lib/http/2/version.rb index 82fa53c..eda1920 100644 --- a/lib/http/2/version.rb +++ b/lib/http/2/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module HTTP2 - VERSION = "1.1.0" + VERSION = "1.1.1" end From 99cdf0808c1411a9d5ef3497f9abe81f2b418b99 Mon Sep 17 00:00:00 2001 From: HoneyryderChuck Date: Wed, 16 Apr 2025 01:07:21 +0100 Subject: [PATCH 7/7] fix permission issue with release id-token param --- .github/workflows/release.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2d3a279..438a35a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -11,6 +11,7 @@ on: permissions: contents: read + id-token: write jobs: release: