Fix some of the proxy code paths

http/h1_stream.lua

@@ -5,6 +5,7 @@ local ce = require "cqueues.errno"
 local new_fifo = require "fifo"
 local lpeg = require "lpeg"
 local http_patts = require "lpeg_patterns.http"
+local uri_patts = require "lpeg_patterns.uri"
 local new_headers = require "http.headers".new
 local reason_phrases = require "http.h1_reason_phrases"
 local stream_common = require "http.stream_common"
@@ -22,6 +23,8 @@ local Connection = lpeg.Ct(http_patts.Connection) * EOF
 local Content_Encoding = lpeg.Ct(http_patts.Content_Encoding) * EOF
 local Transfer_Encoding = lpeg.Ct(http_patts.Transfer_Encoding) * EOF
 local TE = lpeg.Ct(http_patts.TE) * EOF
+local absolute_form = uri_patts.absolute_uri * EOF
+local authority_form = uri_patts.authority * EOF
 
 local function has(list, val)
 	if list then
@@ -74,6 +77,7 @@ local function new_stream(connection)
 
 		req_method = nil; -- string
 		peer_version = nil; -- 1.0 or 1.1
+		use_absolute_target = nil; -- tristate boolean
 		has_main_headers = false;
 		headers_in_progress = nil;
 		headers_fifo = new_fifo();
@@ -249,6 +253,41 @@ function stream_methods:step(timeout)
 	return true
 end
 
+-- should return scheme, authority, path
+local function parse_target(path)
+	if path:sub(1, 1) == "/" or path == "*" then
+		-- 'origin-form' or 'asterisk-form'
+		-- early exit for common case
+		return nil, nil, path
+	end
+
+	local absolute_uri = absolute_form:match(path)
+	if absolute_uri then
+		-- don't want normalised form of authority or path
+		local authority
+		if absolute_uri.host then
+			authority, path = path:match("://([^/]*)(.*)")
+			if path == "" then
+				path = nil
+			end
+		else
+			-- authority is nil
+			-- path should be nil if there are no characters.
+			path = path:match(":(.+)")
+		end
+		return absolute_uri.scheme, authority, path
+	end
+
+	if authority_form:match(path) then
+		-- don't want normalised form of authority
+		-- `path` *is* the authority
+		return nil, path, nil
+	end
+
+	-- other...
+	return nil, nil, path
+end
+
 -- read_headers may be called more than once for a stream
 -- e.g. for 100 Continue
 -- this function *should never throw* under normal operation
@@ -281,12 +320,43 @@ function stream_methods:read_headers(timeout)
 			self.peer_version = httpversion
 			headers = new_headers()
 			headers:append(":method", method)
-			if method == "CONNECT" then
-				headers:append(":authority", target)
-			else
-				headers:append(":path", target)
+			local scheme, authority, path = parse_target(target)
+			if authority then
+				-- RFC 7230 Section 5.4
+				-- When a proxy receives a request with an absolute-form of
+				-- request-target, the proxy MUST ignore the received Host header field
+				-- (if any) and instead replace it with the host information of the
+				-- request-target.
+				headers:append(":authority", authority)
 			end
-			headers:append(":scheme", self:checktls() and "https" or "http")
+			-- RFC 7230 Section 5.5
+			-- If the request-target is in absolute-form, the effective request URI
+			-- is the same as the request-target.  Otherwise, the effective request
+			-- URI is constructed as follows:
+			if not scheme then
+				-- If the server's configuration (or outbound gateway) provides a
+				-- fixed URI scheme, that scheme is used for the effective request
+				-- URI.  Otherwise, if the request is received over a TLS-secured TCP
+				-- connection, the effective request URI's scheme is "https"; if not,
+				-- the scheme is "http".
+				if self:checktls() then
+					scheme = "https"
+				else
+					scheme = "http"
+				end
+			end
+			if path then
+				headers:append(":path", path)
+			elseif method == "OPTIONS" then
+				-- RFC 7230 Section 5.3.4
+				-- If a proxy receives an OPTIONS request with an absolute-form of
+				-- request-target in which the URI has an empty path and no query
+				-- component, then the last proxy on the request chain MUST send a
+				-- request-target of "*" when it forwards the request to the indicated
+				-- origin server.
+				headers:append(":path", "*")
+			end
+			headers:append(":scheme", scheme)
 			self:set_state("open")
 		else -- client
 			-- Make sure we're at front of connection pipeline
@@ -342,9 +412,17 @@ function stream_methods:read_headers(timeout)
 		end
 		k = k:lower() -- normalise to lower case
 		if k == "host" and not is_trailers then
-			k = ":authority"
+			-- RFC 7230 Section 5.4
+			-- When a proxy receives a request with an absolute-form of
+			-- request-target, the proxy MUST ignore the received Host header field
+			-- (if any) and instead replace it with the host information of the
+			-- request-target.
+			if not headers:has(":authority") then
+				headers:append(":authority", v)
+			end
+		else
+			headers:append(k, v)
 		end
-		headers:append(k, v)
 	end
 
 	do
@@ -571,8 +649,32 @@ function stream_methods:write_headers(headers, end_stream, timeout)
 				assert(not headers:has(":path"), "CONNECT requests should not have a path")
 			else
 				-- RFC 7230 Section 5.4: A client MUST send a Host header field in all HTTP/1.1 request messages.
-				assert(self.connection.version < 1.1 or headers:has(":authority"), "missing authority")
+				local has_authority = headers:has(":authority")
+				assert(has_authority or self.connection.version < 1.1, "missing authority")
 				target = assert(headers:get(":path"), "missing path")
+
+				local use_absolute_target = self.use_absolute_target
+				if use_absolute_target then
+					assert(has_authority, "request-target absolute-form requires an authority")
+				elseif use_absolute_target == nil then
+					use_absolute_target = has_authority
+				end
+				if use_absolute_target then
+					-- RFC 7230 Section 5.3.2
+					-- When making a request to a proxy, other than a CONNECT or server-wide
+					-- OPTIONS request (as detailed below), a client MUST send the target
+					-- URI in absolute-form as the request-target.
+					-- ...
+					-- To allow for transition to the absolute-form for all requests in some
+					-- future version of HTTP, a server MUST accept the absolute-form in
+					-- requests, even though HTTP/1.1 clients will only send them in
+					-- requests to proxies.
+					if target == "*" then
+						target = headers:get(":scheme") .. "://" .. headers:get(":authority")
+					else
+						target = headers:get(":scheme") .. "://" .. headers:get(":authority") .. target
+					end
+				end
 			end
 			if self.connection.req_locked then
 				-- Wait until previous request has been fully written
@@ -742,13 +844,9 @@ function stream_methods:write_headers(headers, end_stream, timeout)
 				return nil, err, errno
 			end
 		elseif name == ":authority" then
-			-- for CONNECT requests, :authority is the path
-			if self.req_method ~= "CONNECT" then
-				-- otherwise it's the Host header
-				local ok, err, errno = self.connection:write_header("host", value, 0)
-				if not ok then
-					return nil, err, errno
-				end
+			local ok, err, errno = self.connection:write_header("host", value, 0)
+			if not ok then
+				return nil, err, errno
 			end
 		end
 	end

http/request.lua

@@ -358,6 +358,7 @@ function request_methods:go(timeout)
 	local port = self.port
 	local tls = self.tls
 	local version = self.version
+	local use_absolute_target
 
 	-- RFC 6797 Section 8.3
 	if not tls and self.hsts and self.hsts:check(host) then
@@ -456,17 +457,15 @@ function request_methods:go(timeout)
 				if request_headers:get(":method") == "CONNECT" then
 					error("cannot use HTTP Proxy with CONNECT method")
 				end
-				-- TODO: Check if :path already has authority?
-				local old_url = self:to_uri(false)
 				host = assert(proxy.host, "proxy is missing host")
 				port = proxy.port or http_util.scheme_to_port[proxy.scheme]
-				-- proxy requests get a uri that includes host as their path
-				if not cloned_headers then
-					request_headers = request_headers:clone()
-					cloned_headers = true -- luacheck: ignore 311
-				end
-				request_headers:upsert(":path", old_url)
+				tls = (proxy.scheme == "https")
+				use_absolute_target = true
 				if proxy.userinfo then
+					if not cloned_headers then
+						request_headers = request_headers:clone()
+						cloned_headers = true -- luacheck: ignore 311
+					end
 					request_headers:upsert("proxy-authorization", "basic " .. basexx.to_base64(proxy.userinfo), true)
 				end
 			end
@@ -520,6 +519,10 @@ function request_methods:go(timeout)
 		end
 	end
 
+	if use_absolute_target and connection.version < 2 then
+		stream.use_absolute_target = use_absolute_target
+	end
+
 	local body = self.body
 	do -- Write outgoing headers
 		local ok, err, errno = stream:write_headers(request_headers, body == nil, deadline and deadline-monotime())

spec/h1_stream_spec.lua

@@ -25,6 +25,33 @@ describe("http1 stream", function()
 		assert.same("/", h:get(":path"))
 		assert.same("bar", h:get("foo"))
 	end)
+	it("CONNECT requests should have an host header on the wire", function()
+		local server, client = new_pair(1.1)
+		local cq = cqueues.new()
+		cq:wrap(function()
+			local stream = client:new_stream()
+			local req_headers = new_headers()
+			req_headers:append(":method", "CONNECT")
+			req_headers:append(":scheme", "http")
+			req_headers:append(":authority", "myauthority:8888")
+			assert(stream:write_headers(req_headers, true))
+			stream:shutdown()
+		end)
+		cq:wrap(function()
+			local method, path, httpversion = assert(server:read_request_line())
+			assert.same("CONNECT", method)
+			assert.same("myauthority:8888", path)
+			assert.same(1.1, httpversion)
+			local k, v = assert(server:read_header())
+			assert.same("host", k)
+			assert.same("myauthority:8888", v)
+			server:shutdown()
+		end)
+		assert_loop(cq, TEST_TIMEOUT)
+		assert.truthy(cq:empty())
+		server:close()
+		client:close()
+	end)
 	it("Writing to a shutdown connection returns EPIPE", function()
 		local server, client = new_pair(1.1)
 		local stream = client:new_stream()

spec/request_spec.lua

@@ -611,8 +611,9 @@ describe("http.request module", function()
 				local h = assert(stream:get_headers())
 				local _, host, port = stream:localname()
 				local authority = http_util.to_authority(host, port, "http")
+				assert.same("http", h:get(":scheme"))
 				assert.same(authority, h:get ":authority")
-				assert.same("http://" .. authority .. "/", h:get(":path"))
+				assert.same("/", h:get(":path"))
 				local resp_headers = new_headers()
 				resp_headers:append(":status", "200")
 				assert(stream:write_headers(resp_headers, false))
@@ -629,13 +630,38 @@ describe("http.request module", function()
 				stream:shutdown()
 			end)
 		end)
-		it("works with a proxy server with a path component", function()
+		it("works with a tls proxy server", function()
 			test(function(stream)
 				local h = assert(stream:get_headers())
+				assert.truthy(stream:checktls()) -- came in via TLS
 				local _, host, port = stream:localname()
 				local authority = http_util.to_authority(host, port, "http")
+				assert.same("http", h:get(":scheme"))
 				assert.same(authority, h:get ":authority")
-				assert.same("http://" .. authority .. "/", h:get(":path"))
+				assert.same("/", h:get(":path"))
+				local resp_headers = new_headers()
+				resp_headers:append(":status", "200")
+				assert(stream:write_headers(resp_headers, false))
+				assert(stream:write_chunk("hello world", true))
+			end, function(req)
+				req.proxy = {
+					scheme = "https";
+					host = req.host;
+					port = req.port;
+				}
+				local headers, stream = assert(req:go())
+				assert.same("200", headers:get(":status"))
+				assert.same("hello world", assert(stream:get_body_as_string()))
+				stream:shutdown()
+			end)
+		end)
+		it("works with a proxy server with a path component", function()
+			test(function(stream)
+				local h = assert(stream:get_headers())
+				local _, host, port = stream:localname()
+				assert.same("http", h:get(":scheme"))
+				assert.same(http_util.to_authority(host, port, "http"), h:get ":authority")
+				assert.same("/", h:get(":path"))
 				local resp_headers = new_headers()
 				resp_headers:append(":status", "200")
 				assert(stream:write_headers(resp_headers, false))
@@ -658,7 +684,9 @@ describe("http.request module", function()
 				local h = assert(stream:get_headers())
 				assert.same("OPTIONS", h:get ":method")
 				local _, host, port = stream:localname()
-				assert.same("http://" .. http_util.to_authority(host, port, "http"), h:get(":path"))
+				assert.same("http", h:get(":scheme"))
+				assert.same(http_util.to_authority(host, port, "http"), h:get(":authority"))
+				assert.same("*", h:get(":path"))
 				stream:shutdown()
 			end, function(req)
 				req.headers:upsert(":method", "OPTIONS")
@@ -693,9 +721,9 @@ describe("http.request module", function()
 			test(function(stream)
 				local h = assert(stream:get_headers())
 				local _, host, port = stream:localname()
-				local authority = http_util.to_authority(host, port, "http")
-				assert.same(authority, h:get ":authority")
-				assert.same("http://" .. authority .. "/foo", h:get(":path"))
+				assert.same("http", h:get(":scheme"))
+				assert.same(http_util.to_authority(host, port, "http"), h:get ":authority")
+				assert.same("/foo", h:get(":path"))
 				stream:shutdown()
 			end, function(req)
 				req.proxy = {