Allow not validating target #456

PragTob · 2024-11-08T10:16:59Z

As discussed in #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! 💚

Tasks left for me:

Check if this actually fixes what I reported in Allow not validating target #453 :)

PragTob · 2024-11-08T10:18:01Z

lib/mint/http1.ex

+    * `: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.?.?*


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? 🤔

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

coveralls · 2024-11-08T10:18:50Z

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%

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

💛 - Coveralls

PragTob · 2024-11-08T10:19:39Z

lib/mint/http1/request.ex

@@ -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


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.

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?

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!

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 !

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

test/mint/http1/conn_test.exs

PragTob · 2024-11-08T10:21:36Z

test/mint/http1/conn_test.exs

@@ -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()}


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!

As discussed in elixir-mint#453 I tried to follow the guidance of `case_sensitive_headers` so that these options are treated somewhat similarly.

PragTob · 2024-11-08T10:23:18Z

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

PragTob · 2024-11-08T11:04:11Z

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 🎉

whatyouhide

Looking already great 💟

lib/mint/http1.ex

whatyouhide · 2024-11-08T12:00:43Z

lib/mint/http1.ex

+    * `: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.?.?*


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

whatyouhide · 2024-11-08T12:10:17Z

lib/mint/http1/request.ex

@@ -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


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?

test/mint/http1/conn_test.exs

Co-authored-by: Andrea Leopardi <[email protected]>

Ref: elixir-mint#456 (comment)

whatyouhide · 2024-11-09T11:26:10Z

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

PragTob · 2024-11-11T08:31:16Z

@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 · 2024-11-11T08:48:14Z

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

path is taken here:

mint/lib/mint/http2.ex

Lines 510 to 527 in 8516bcd

    
           def request(%__MODULE__{} = conn, method, path, headers, body) 
        
               when is_binary(method) and is_binary(path) and is_list(headers) do 
        
             headers = 
        
               headers 
        
               |> Headers.lower_raws() 
        
               |> add_pseudo_headers(conn, method, path) 
        
               |> add_default_headers(body) 
        
               |> sort_pseudo_headers_to_front() 
        
             {conn, stream_id, ref} = open_stream(conn) 
        
             {conn, payload} = encode_request_payload(conn, stream_id, headers, body) 
        
             conn = send!(conn, payload) 
        
             {:ok, conn, ref} 
        
           catch 
        
             :throw, {:mint, _conn, reason} -> 
        
               # The stream is invalid and "_conn" may be tracking it, so we return the original connection instead. 
        
               {:error, conn, reason} 
        
           end

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

mint/lib/mint/http2.ex

Lines 1354 to 1370 in 8516bcd

    
           defp add_pseudo_headers(headers, conn, method, path) do 
        
             if same_method?(method, "CONNECT") do 
        
               [ 
        
                 {":method", method}, 
        
                 {":authority", conn.authority} 
        
                 | headers 
        
               ] 
        
             else 
        
               [ 
        
                 {":method", method}, 
        
                 {":path", path}, 
        
                 {":scheme", conn.scheme}, 
        
                 {":authority", conn.authority} 
        
                 | headers 
        
               ] 
        
             end 
        
           end

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 · 2024-11-11T09:16:34Z

Coolsies, then we're good 🙃

PragTob commented Nov 8, 2024

View reviewed changes

test/mint/http1/conn_test.exs Show resolved Hide resolved

PragTob commented Nov 8, 2024

View reviewed changes

PragTob added 2 commits November 8, 2024 11:22

Introduce new HTTP1 option: skip_target_validation

404a702

As discussed in elixir-mint#453 I tried to follow the guidance of `case_sensitive_headers` so that these options are treated somewhat similarly.

Centralize mint_user_agent in tests

573a863

PragTob force-pushed the allow-not-validating-target branch from 2adaf60 to 573a863 Compare November 8, 2024 10:22

whatyouhide reviewed Nov 8, 2024

View reviewed changes

PragTob and others added 3 commits November 8, 2024 13:32

Update lib/mint/http1.ex

f8fbd26

Co-authored-by: Andrea Leopardi <[email protected]>

Use module attribute for mint_user_agent

5f2e805

Move target validation to Mint.HTTP1

6ec9faa

Ref: elixir-mint#456 (comment)

whatyouhide merged commit 8516bcd into elixir-mint:main Nov 9, 2024
3 checks passed

PragTob deleted the allow-not-validating-target branch November 11, 2024 08:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow not validating target #456

Allow not validating target #456

PragTob commented Nov 8, 2024 •

edited

Loading

PragTob Nov 8, 2024

whatyouhide Nov 8, 2024

coveralls commented Nov 8, 2024 •

edited

Loading

PragTob Nov 8, 2024

whatyouhide Nov 8, 2024

PragTob Nov 8, 2024

PragTob Nov 8, 2024

whatyouhide Nov 9, 2024

PragTob Nov 8, 2024

PragTob commented Nov 8, 2024

PragTob commented Nov 8, 2024 •

edited

Loading

whatyouhide left a comment

whatyouhide Nov 8, 2024

whatyouhide Nov 8, 2024

whatyouhide commented Nov 9, 2024

PragTob commented Nov 11, 2024

PragTob commented Nov 11, 2024

whatyouhide commented Nov 11, 2024

Allow not validating target #456

Allow not validating target #456

Conversation

PragTob commented Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 8, 2024 • edited Loading

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

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob commented Nov 8, 2024

PragTob commented Nov 8, 2024 • edited Loading

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whatyouhide commented Nov 9, 2024

PragTob commented Nov 11, 2024

PragTob commented Nov 11, 2024

whatyouhide commented Nov 11, 2024

PragTob commented Nov 8, 2024 •

edited

Loading

coveralls commented Nov 8, 2024 •

edited

Loading

PragTob commented Nov 8, 2024 •

edited

Loading