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.

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

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

Open
joeapearson opened this issue Mar 15, 2021 · 4 comments
Open

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

joeapearson opened this issue Mar 15, 2021 · 4 comments

Comments

@joeapearson
Copy link

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
Copy link
Author

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:

  • Update EctoFields.URL to accept all valid URLs. AFAIK this is much more involved than it sounds. But there might be some work that could be borrowed from https://mathiasbynens.be/demo/url-regex for example.
  • Update EctoFields.URL to accept some sort of per-field configuration allowing a user to customise the validation behaviour of URLs.

@sitch
Copy link

sitch commented Apr 30, 2021

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
Copy link
Owner

jerel commented Nov 22, 2021

I've added support for ports and IP addresses via these commits 7d39c88...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
Copy link

roylez commented Feb 28, 2024

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:[email protected]")
:error
> URI.parse("https://a:[email protected]")
%URI{
  scheme: "https",
  authority: "a:[email protected]",
  userinfo: "a:b",
  host: "google.com",
  port: 443,
  path: nil,
  query: nil,
  fragment: nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants