Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow not validating target #456

Merged
merged 5 commits into from
Nov 9, 2024
Merged

Allow not validating target #456

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
404a702
Introduce new HTTP1 option: skip_target_validation
PragTob Nov 8, 2024
573a863
Centralize mint_user_agent in tests
PragTob Nov 8, 2024
f8fbd26
Update lib/mint/http1.ex
PragTob Nov 8, 2024
5f2e805
Use module attribute for `mint_user_agent`
PragTob Nov 8, 2024
6ec9faa
Move target validation to `Mint.HTTP1`
PragTob Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lib/mint/http1.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ defmodule Mint.HTTP1 do
:mode,
:scheme_as_string,
:case_sensitive_headers,
:skip_target_validation,
requests: :queue.new(),
state: :closed,
buffer: "",
Expand Down Expand Up @@ -123,6 +124,10 @@ defmodule Mint.HTTP1 do
* `:case_sensitive_headers` - (boolean) if set to `true` the case of the supplied
headers in requests will be preserved. The default is to lowercase the headers
because HTTP/1.1 header names are case-insensitive. *Available since v1.6.0*.
* `:skip_target_validation` - (boolean) if set to `true` the target of a request
will not be validated. You might want this if you deal with non standard-
conform URIs but need to preserve them. The default is to validate the request
target. *Available since v1.?.?*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it to keep on par with case_sensititve_headers but I'm not sure if this would be v 1.7.0 if it was released (technically it's a feature).

Also, should I add it to the Changelog then? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.7.0 is fine, but we will add it to the changelog when cutting a release.

PragTob marked this conversation as resolved.
Show resolved Hide resolved

"""
@spec connect(Types.scheme(), Types.address(), :inet.port_number(), keyword()) ::
Expand Down Expand Up @@ -200,7 +205,8 @@ defmodule Mint.HTTP1 do
scheme_as_string: Atom.to_string(scheme),
state: :open,
log: log?,
case_sensitive_headers: Keyword.get(opts, :case_sensitive_headers, false)
case_sensitive_headers: Keyword.get(opts, :case_sensitive_headers, false),
skip_target_validation: Keyword.get(opts, :skip_target_validation, false)
}

{:ok, conn}
Expand Down Expand Up @@ -280,7 +286,8 @@ defmodule Mint.HTTP1 do
method,
path,
Headers.to_raw(headers, conn.case_sensitive_headers),
body
body,
conn.skip_target_validation
),
:ok <- transport.send(socket, iodata) do
request_ref = make_ref()
Expand Down
8 changes: 4 additions & 4 deletions lib/mint/http1/request.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ defmodule Mint.HTTP1.Request do

import Mint.HTTP1.Parse

def encode(method, target, headers, body) do
def encode(method, target, headers, body, skip_target_validation \\ false) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this duplicates the default but I didn't:

  • want it to be forced to be passed along
  • break the existing interface (although technically it's @moduledoc false)

Also opted for a plain boolean, could also be turned into an opts/kwlist but seemed simpler so far.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's enforce this argument, this is a private interface. Also, if we can, we could move the call to validate_target!/1 over to Mint.HTTP, before calling Request.encode. That way, Request doesn't even need to know about the new option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try moving it over, the case sensitive headers option is also handled purely in Mint.HTTP - wasn't sure about the separation of concerns. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it over after a short coffee break ☕

One thing that is slightly odd now is that target validation is in Mint.HTTP1 while header validation is still in Mint.HTTP1.Request:

  defp encode_headers(headers) do
    Enum.reduce(headers, "", fn {name, value}, acc ->
      validate_header_name!(name)
      validate_header_value!(name, value)
      [acc, name, ": ", value, "\r\n"]
    end)
  end

Might be worth to move. The reason I didn't yet is because it'd mean enumerating the headers another time and I'm not sure if that's a performance concern that matters in mints context (generally it shouldn't matter for "normal" amounts of headers but yeah).
Also it'd mean me changing more code that I don't know super well :)

Happy to change or keep depending on what you think @whatyouhide !

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it this way, don't worry about it for now.

body = [
encode_request_line(method, target),
encode_request_line(method, target, skip_target_validation),
encode_headers(headers),
"\r\n",
encode_body(body)
Expand All @@ -16,8 +16,8 @@ defmodule Mint.HTTP1.Request do
{:mint, reason} -> {:error, reason}
end

defp encode_request_line(method, target) do
validate_target!(target)
defp encode_request_line(method, target, skip_target_validation) do
unless skip_target_validation, do: validate_target!(target)
[method, ?\s, target, " HTTP/1.1\r\n"]
end

Expand Down
58 changes: 46 additions & 12 deletions test/mint/http1/conn_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a small convenience change I made in a separate commit - hope it's not too much noise but I also didn't wanna do it in a separate PR. Happy to roll it back!


\
""")
Expand All @@ -570,7 +570,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

\
""")
Expand All @@ -591,7 +591,7 @@ defmodule Mint.HTTP1Test do
GET / HTTP/1.1
content-length: 4
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

body\
""")
Expand All @@ -607,7 +607,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

\
""")
Expand All @@ -626,7 +626,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
content-length: 10

body\
Expand Down Expand Up @@ -760,6 +760,38 @@ defmodule Mint.HTTP1Test do

""")
end

@invalid_target "%%"
test "targets are validated by default", %{port: port, server_ref: server_ref} do
assert {:ok, conn} = HTTP1.connect(:http, "localhost", port)
PragTob marked this conversation as resolved.
Show resolved Hide resolved

assert_receive {^server_ref, _server_socket}

assert {:error, %Mint.HTTP1{},
%Mint.HTTPError{reason: {:invalid_request_target, @invalid_target}}} =
HTTP1.request(conn, "GET", @invalid_target, [], "")
end

test "target validation may be skipped based on connection options", %{
port: port,
server_ref: server_ref
} do
assert {:ok, conn} = HTTP1.connect(:http, "localhost", port, skip_target_validation: true)

assert_receive {^server_ref, server_socket}

assert {:ok, _conn, _ref} = HTTP1.request(conn, "GET", @invalid_target, [], "body")

assert receive_request_string(server_socket) ==
request_string("""
GET %% HTTP/1.1
content-length: 4
host: localhost:#{port}
user-agent: #{mint_user_agent()}

body\
""")
end
end

describe "streaming requests" do
Expand All @@ -772,7 +804,7 @@ defmodule Mint.HTTP1Test do
GET / HTTP/1.1
transfer-encoding: chunked
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

\
""")
Expand All @@ -795,7 +827,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
transfer-encoding: chunked

\
Expand All @@ -815,7 +847,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
transfer-encoding: gzip,chunked

\
Expand All @@ -839,7 +871,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
transfer-encoding: identity

\
Expand All @@ -859,7 +891,7 @@ defmodule Mint.HTTP1Test do
request_string("""
GET / HTTP/1.1
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}
content-length: 5

\
Expand All @@ -877,7 +909,7 @@ defmodule Mint.HTTP1Test do
GET / HTTP/1.1
transfer-encoding: chunked
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

\
""")
Expand All @@ -897,7 +929,7 @@ defmodule Mint.HTTP1Test do
POST / HTTP/1.1
transfer-encoding: chunked
host: localhost:#{port}
user-agent: mint/#{Mix.Project.config()[:version]}
user-agent: #{mint_user_agent()}

\
""")
Expand Down Expand Up @@ -997,4 +1029,6 @@ defmodule Mint.HTTP1Test do
defp stream_message_bytewise(<<>>, conn, responses) do
{:ok, conn, responses}
end

defp mint_user_agent, do: "mint/#{Mix.Project.config()[:version]}"
PragTob marked this conversation as resolved.
Show resolved Hide resolved
end
14 changes: 11 additions & 3 deletions test/mint/http1/request_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ defmodule Mint.HTTP1.RequestTest do
""")
end

@invalid_request_targets ["/ /", "/%foo", "/foo%x"]
test "validates request target" do
for invalid_target <- ["/ /", "/%foo", "/foo%x"] do
for invalid_target <- @invalid_request_targets do
assert Request.encode("GET", invalid_target, [], nil) ==
{:error, {:invalid_request_target, invalid_target}}
end
Expand All @@ -43,6 +44,13 @@ defmodule Mint.HTTP1.RequestTest do
assert String.starts_with?(request, request_string("GET /foo%20bar HTTP/1.1"))
end

test "can optionally skip validating the request target" do
for invalid_target <- @invalid_request_targets do
request = encode_request("GET", invalid_target, [], nil, true)
assert String.starts_with?(request, request_string("GET #{invalid_target} HTTP/1.1"))
end
end

test "invalid header name" do
assert Request.encode("GET", "/", [{"f oo", "bar"}], nil) ==
{:error, {:invalid_header_name, "f oo"}}
Expand Down Expand Up @@ -76,8 +84,8 @@ defmodule Mint.HTTP1.RequestTest do
end
end

defp encode_request(method, target, headers, body) do
assert {:ok, iodata} = Request.encode(method, target, headers, body)
defp encode_request(method, target, headers, body, skip_target_validation \\ false) do
assert {:ok, iodata} = Request.encode(method, target, headers, body, skip_target_validation)
IO.iodata_to_binary(iodata)
end

Expand Down
Loading