Allow not validating target

PragTob commented 2 weeks ago

As discussed in https://github.com/elixir-mint/mint/issues/453

I tried to follow the guidance of case_sensitive_headers so that these options are treated somewhat similarly.

I tried to keep with the patterns of surrounding code, if you want anything changed/I missed something - happy to do so!

In a separate commit I also included a small nicety as I saw the definition of the mint user agent duplicated over the tests. Happy to roll that back though.

Some questions/remarks inline.

Thanks for all your work! :green_heart:

Tasks left for me:

[ ] Check if this actually fixes what I reported in #453 :)

IMG_20220619_184733

coveralls commented 2 weeks ago

Pull Request Test Coverage Report for Build 6ec9faaaab89127f26cfdc07c14d49992b33d7ce-PR-456

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 87.423%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/mint/http1.ex	7	8	87.5%
<!--	Total:	7	8	87.5%	-->

Totals
Change from base Build 5e9eac16f6e6d8211afe526043e6fab9a2ff869d:	0.02%
Covered Lines:	1279
Relevant Lines:	1463

💛 - Coveralls

PragTob commented 2 weeks ago

Sorry for the force push, forgot one guiding comment and wanted to have it gone form history :sweat_smile:

PragTob commented 2 weeks ago

Works as expected on the mint level:

iex(1)> {:ok, conn} = Mint.HTTP.connect(:http, "httpbin.org", 80); {:ok, conn, request_ref} = Mint.HTTP.request(conn, "GET", "?na=%%boom%%", [], "")
** (MatchError) no match of right hand side value: {:error, %Mint.HTTP1{host: "httpbin.org", port: 80, request: nil, streaming_request: nil, socket: #Port<0.5>, transport: Mint.Core.Transport.TCP, mode: :active, scheme_as_string: "http", case_sensitive_headers: false, skip_target_validation: false, requests: {[], []}, state: :open, buffer: "", proxy_headers: [], private: %{}, log: false}, %Mint.HTTPError{reason: {:invalid_request_target, "?na=%%boom%%"}, module: Mint.HTTP1}}
    (stdlib 6.1) erl_eval.erl:652: :erl_eval.expr/6
    iex:1: (file)
iex(1)> {:ok, conn2} = Mint.HTTP.connect(:http, "httpbin.org", 80, skip_target_validation: true); {:ok, conn, request_ref} = Mint.HTTP.request(conn2, "GET", "?na=%%boom%%", [], "")
{:ok,
 %Mint.HTTP1{
   host: "httpbin.org",
   port: 80,
   request: %{
     status: nil,
     version: nil,
     state: :status,
     connection: [],
     body: nil,
     ref: #Reference<0.1432147674.813432838.138316>,
     data_buffer: [],
     headers_buffer: [],
     content_length: nil,
     method: "GET",
     transfer_encoding: []
   },
   streaming_request: nil,
   socket: #Port<0.6>,
   transport: Mint.Core.Transport.TCP,
   mode: :active,
   scheme_as_string: "http",
   case_sensitive_headers: false,
   skip_target_validation: true,
   requests: {[], []},
   state: :open,
   buffer: "",
   proxy_headers: [],
   private: %{},
   log: false
 },
 #Reference<0.1432147674.813432838.138316>}
iex(2)> flush()
{:tcp, #Port<0.6>,
 "HTTP/1.1 400 Bad Request\r\nServer: awselb/2.0\r\nDate: Fri, 08 Nov 2024 11:01:57 GMT\r\nContent-Type: text/html\r\nContent-Length: 122\r\nConnection: close\r\n\r\n<html>\r\n<head><title>400 Bad Request</title></head>\r\n<body>\r\n<center><h1>400 Bad Request</h1></center>\r\n</body>\r\n</html>\r\n"}
:ok

~~Can't figure out how to get it through Req/Finch (I thought I could get them to my Finch pool via conn_opts but that seems to not work, but that's not for here to solve :) )~~ (classical user error)

edit:

Got it working with Req/Finch:

iex(1)>  Req.get!("http://example.com?invalid=%%target%%", finch: MyPool)
%Req.Response{
  status: 200,
  headers: %{
    "accept-ranges" => ["bytes"],
    "age" => ["324892"],
    "cache-control" => ["max-age=604800"],
    "content-type" => ["text/html; charset=UTF-8"],
    "date" => ["Fri, 08 Nov 2024 11:30:37 GMT"],
    "etag" => ["\"3147526947\""],
    "expires" => ["Fri, 15 Nov 2024 11:30:37 GMT"],
    "last-modified" => ["Thu, 17 Oct 2019 07:18:26 GMT"],
    "server" => ["ECAcc (nyd/D130)"],
    "vary" => ["Accept-Encoding"],
    "x-cache" => ["HIT"]
  },
  body: ...
}

cc: @wojtekmach :tada:

whatyouhide commented 1 week ago

@PragTob do we need to do this for HTTP/2 too?

PragTob commented 1 week ago

@whatyouhide I'm unsure :) I'm guessing so but I didn't find any similar code with HTTP2 on a cursory look and I'm also not as familiar with HTTP2, let me see if I can find something.

PragTob commented 1 week ago

@whatyouhide as best as I can see there is no validation going on in the HTTP2 implementation.

path is taken here: https://github.com/elixir-mint/mint/blob/8516bcda190a644a5fe960b13869107ff76ee9b3/lib/mint/http2.ex#L510-L527

and only passed on to this function where it is just added to the header:

https://github.com/elixir-mint/mint/blob/8516bcda190a644a5fe960b13869107ff76ee9b3/lib/mint/http2.ex#L1354-L1370

there are some further validations going on, but as far as I can see none of them are for the path in particular/for a similar problem.

whatyouhide commented 1 week ago

Coolsies, then we're good 🙃

elixir-mint / mint