Skip to content

Commit f196b23

Browse files
evanphxdentarg
authored and
nateberkopec
committed
Merge commit from fork
* Prevent underscores from clobbering hyphen headers * Special case encoding headers to prevent app confusion * Handle _ as , in jruby as well * Silence RuboCop offense --------- Co-authored-by: Patrik Ragnarsson <[email protected]>
1 parent 24eec19 commit f196b23

File tree

5 files changed

+111
-3
lines changed

5 files changed

+111
-3
lines changed

‎ext/puma_http11/org/jruby/puma/Http11.java

+2
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,8 @@ public static void http_field(Ruby runtime, RubyHash req, ByteList buffer, int f
9999
int bite = b.get(i) & 0xFF;
100100
if(bite == '-') {
101101
b.set(i, (byte)'_');
102+
} else if(bite == '_') {
103+
b.set(i, (byte)',');
102104
} else {
103105
b.set(i, (byte)Character.toUpperCase(bite));
104106
}

‎lib/puma/const.rb

+8
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,14 @@ module Const
244244
# header values can contain HTAB?
245245
ILLEGAL_HEADER_VALUE_REGEX = /[\x00-\x08\x0A-\x1F]/.freeze
246246

247+
# The keys of headers that should not be convert to underscore
248+
# normalized versions. These headers are ignored at the request reading layer,
249+
# but if we normalize them after reading, it's just confusing for the application.
250+
UNMASKABLE_HEADERS = {
251+
"HTTP_TRANSFER,ENCODING" => true,
252+
"HTTP_CONTENT,LENGTH" => true,
253+
}
254+
247255
# Banned keys of response header
248256
BANNED_HEADER_KEY = /\A(rack\.|status\z)/.freeze
249257

‎lib/puma/request.rb

+16-3
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,11 @@ def illegal_header_value?(header_value)
318318
# compatibility, we'll convert them back. This code is written to
319319
# avoid allocation in the common case (ie there are no headers
320320
# with `,` in their names), that's why it has the extra conditionals.
321+
#
322+
# @note If a normalized version of a `,` header already exists, we ignore
323+
# the `,` version. This prevents clobbering headers managed by proxies
324+
# but not by clients (Like X-Forwarded-For).
325+
#
321326
# @param env [Hash] see Puma::Client#env, from request, modifies in place
322327
# @version 5.0.3
323328
#
@@ -326,23 +331,31 @@ def req_env_post_parse(env)
326331
to_add = nil
327332

328333
env.each do |k,v|
329-
if k.start_with?("HTTP_") and k.include?(",") and k != "HTTP_TRANSFER,ENCODING"
334+
if k.start_with?("HTTP_") && k.include?(",") && !UNMASKABLE_HEADERS.key?(k)
330335
if to_delete
331336
to_delete << k
332337
else
333338
to_delete = [k]
334339
end
335340

341+
new_k = k.tr(",", "_")
342+
if env.key?(new_k)
343+
next
344+
end
345+
336346
unless to_add
337347
to_add = {}
338348
end
339349

340-
to_add[k.tr(",", "_")] = v
350+
to_add[new_k] = v
341351
end
342352
end
343353

344-
if to_delete
354+
if to_delete # rubocop:disable Style/SafeNavigation
345355
to_delete.each { |k| env.delete(k) }
356+
end
357+
358+
if to_add
346359
env.merge! to_add
347360
end
348361
end

‎test/test_normalize.rb

+57
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# frozen_string_literal: true
2+
3+
require_relative "helper"
4+
5+
require "puma/request"
6+
7+
class TestNormalize < Minitest::Test
8+
parallelize_me!
9+
10+
include Puma::Request
11+
12+
def test_comma_headers
13+
env = {
14+
"HTTP_X_FORWARDED_FOR" => "1.1.1.1",
15+
"HTTP_X_FORWARDED,FOR" => "2.2.2.2",
16+
}
17+
18+
req_env_post_parse env
19+
20+
expected = {
21+
"HTTP_X_FORWARDED_FOR" => "1.1.1.1",
22+
}
23+
24+
assert_equal expected, env
25+
26+
# Test that the iteration order doesn't matter
27+
28+
env = {
29+
"HTTP_X_FORWARDED,FOR" => "2.2.2.2",
30+
"HTTP_X_FORWARDED_FOR" => "1.1.1.1",
31+
}
32+
33+
req_env_post_parse env
34+
35+
expected = {
36+
"HTTP_X_FORWARDED_FOR" => "1.1.1.1",
37+
}
38+
39+
assert_equal expected, env
40+
end
41+
42+
def test_unmaskable_headers
43+
env = {
44+
"HTTP_CONTENT,LENGTH" => "100000",
45+
"HTTP_TRANSFER,ENCODING" => "chunky"
46+
}
47+
48+
req_env_post_parse env
49+
50+
expected = {
51+
"HTTP_CONTENT,LENGTH" => "100000",
52+
"HTTP_TRANSFER,ENCODING" => "chunky"
53+
}
54+
55+
assert_equal expected, env
56+
end
57+
end

‎test/test_request_invalid.rb

+28
Original file line numberDiff line numberDiff line change
@@ -216,4 +216,32 @@ def test_chunked_size_mismatch_2
216216

217217
assert_status data
218218
end
219+
220+
def test_underscore_header_1
221+
hdrs = [
222+
"X-FORWARDED-FOR: 1.1.1.1", # proper
223+
"X-FORWARDED-FOR: 2.2.2.2", # proper
224+
"X_FORWARDED-FOR: 3.3.3.3", # invalid, contains underscore
225+
"Content-Length: 5",
226+
].join "\r\n"
227+
228+
response = send_http_and_read "#{GET_PREFIX}#{hdrs}\r\n\r\nHello\r\n\r\n"
229+
230+
assert_includes response, "HTTP_X_FORWARDED_FOR = 1.1.1.1, 2.2.2.2"
231+
refute_includes response, "3.3.3.3"
232+
end
233+
234+
def test_underscore_header_2
235+
hdrs = [
236+
"X_FORWARDED-FOR: 3.3.3.3", # invalid, contains underscore
237+
"X-FORWARDED-FOR: 2.2.2.2", # proper
238+
"X-FORWARDED-FOR: 1.1.1.1", # proper
239+
"Content-Length: 5",
240+
].join "\r\n"
241+
242+
response = send_http_and_read "#{GET_PREFIX}#{hdrs}\r\n\r\nHello\r\n\r\n"
243+
244+
assert_includes response, "HTTP_X_FORWARDED_FOR = 2.2.2.2, 1.1.1.1"
245+
refute_includes response, "3.3.3.3"
246+
end
219247
end

0 commit comments

Comments
 (0)