jerel / ecto_fields

Provides common fields for Ecto
MIT License
43 stars 7 forks source link

EctoFields.URL does not support URLs with ports in them #10

Open joeapearson opened 3 years ago

joeapearson commented 3 years ago

For example:

EctoFields.URL.cast("http://example.com:1234/")
:error

Problem appears to be at https://github.com/jerel/ecto_fields/blob/master/lib/fields/url.ex#L60

joeapearson commented 3 years ago

Actually, looking at the code it seems like there are probably a great deal of valid URLs that wouldn't be correctly treated. However what EctoFields is doing is a sensible default for a great deal of use cases, was that the original intention?

Suitable fixes that I can think of:

sitch commented 3 years ago

I used something like this:

  @spec valid_url?(any()) :: boolean()
  def valid_url?(url) when is_binary(url) do
    case URI.parse(url) do
      %URI{host: nil} -> false
      %URI{scheme: nil} -> false
      %URI{} -> true
    end
  end

  def valid_url?(_), do: false
jerel commented 2 years ago

I've added support for ports and IP addresses via these commits https://github.com/jerel/ecto_fields/compare/7d39c880a68e897bad0d89e97ac0ef0078fe1221...master

@joeapearson Yes, the idea is to have sensible handling for most use cases. In your estimation is the current implementation too restrictive or too permissive?

roylez commented 6 months ago

I feel it is too restrictive. If URI.parse does not complain, why Ecto.Fields should?

Apparently it does not allow simply authentication details in url right now.

> EctoFields.URL.cast("http://google.com")
{:ok, "http://google.com"}
> EctoFields.URL.cast("http://a:b@google.com")
:error
> URI.parse("https://a:b@google.com")
%URI{
  scheme: "https",
  authority: "a:b@google.com",
  userinfo: "a:b",
  host: "google.com",
  port: 443,
  path: nil,
  query: nil,
  fragment: nil
}