Repository: phoenixframework/phoenix_html
Pull request #172 (2017-09-01 21:29:44)

Whitelist accepted URI schemes
This is a PR to allow conversation to continue, but isn't yet complete. The current implementation only raises for "link", but I suspect we will also want to do so for "button"?  

I went with a [whitelist approach](https://security.stackexchange.com/questions/148428/which-url-schemes-are-dangerous-xss-exploitable). And I also added a start to the tuples allowing javascript or data URIs. It is currently only:

```
defp invalid_destination?(to) when is_tuple(to), do: false
```

Do we want to implement the functionality to transform `{:javascript, val}` within the `content_tag` function? Or do we want the `link` function to transform that value?

Edit:

I do think transforming w/in link could be nice, because that would allow us to clear the invalid URL instead of raising (which becomes a DoS as opposed to an XSS).

Something to the effect of...

```
...
to = dest(to)
...
for scheme <- @valid_uri_schemes do
  defp dest(unquote(scheme) <> _ = to), do: to
end
defp dest({scheme, to}) when is_atom(scheme) do
  if String.starts_with?(to, "#{scheme}:"), do: to
end
defp dest(to) when is_binary(to) do
  if !String.contains?(to, ":"), do: to
end
... 
```



GriffinMB: This PR should address all issues raised with the previous PR! Let me know if any other changes are needed.
josevalim: @GriffinMB I have added some final comments and we should be almost there. See you in Seattle!
GriffinMB: Now nested schemes like `link("foo", to: {:javascript, {:safe, "alert(1)"}})` or `link("foo", to: {:safe, {:javascript, "alert(1)"}})` will work properly. And anything in the scheme tuple format will pass the invalid_destination check, just like standard schemes. 

Something like {:bitcoin, {:javascript, "alert(1)"}} will raise the following error: "lists in Phoenix.HTML and templates may only contain integers representing bytes, binaries or other lists". I thought this made more sense than adding a guard to `valid_destination!` and throwing an "unsupported scheme" error. But I'm open to the idea that that is the wrong choice!

Patch:
diff --git a/lib/phoenix_html/link.ex b/lib/phoenix_html/link.ex
index 56b91aa..6b665d8 100644
--- a/lib/phoenix_html/link.ex
+++ b/lib/phoenix_html/link.ex
@@ -66,6 +66,11 @@ defmodule Phoenix.HTML.Link do
       config :phoenix_html, csrf_token_generator: {MyGenerator, :get_token, []}
 
   """
+  @valid_uri_schemes ["http:", "https:", "ftp:", "ftps:", "mailto:",
+                      "news:", "irc:", "gopher:", "nntp:", "feed:",
+                      "telnet:", "mms:", "rtsp:", "svn:", "tel:", "fax:",
+                      "xmpp:"]
+
   def link(text, opts)
 
   def link(opts, do: contents) when is_list(opts) do
@@ -78,6 +83,9 @@ defmodule Phoenix.HTML.Link do
 
   def link(text, opts) do
     {to, opts} = pop_required_option!(opts, :to, "expected non-nil value for :to in link/2")
+
+    to = valid_destination!(to, "link/2")
+
     {method, opts} = Keyword.pop(opts, :method, :get)
 
     if method == :get do
@@ -129,6 +137,8 @@ defmodule Phoenix.HTML.Link do
     {to, opts} = pop_required_option!(opts, :to, "option :to is required in button/2")
     {method, opts} = Keyword.pop(opts, :method, :post)
 
+    to = valid_destination!(to, "button/2")
+
     if method == :get do
       opts = skip_csrf(opts)
       content_tag(:button, text, [data: [method: method, to: to]] ++ opts)
@@ -165,4 +175,33 @@ defmodule Phoenix.HTML.Link do
 
     {value, opts}
   end
+
+  defp valid_destination!(to, calling_func) do
+    if invalid_destination?(to) do
+      raise ArgumentError, """
+      unsupported scheme given to #{calling_func}. In case you want to link to an
+      unknown or unsafe scheme, such as javascript, use a tuple: {:javascript, rest}
+      """
+    end
+
+    valid_destination!(to)
+  end
+  defp valid_destination!({:safe, to}) do
+    {:safe, valid_destination!(to)}
+  end
+  defp valid_destination!({other, to}) when is_atom(other) do
+    [Atom.to_string(other), ?:, to]
+  end
+  defp valid_destination!(to), do: to
+
+  for scheme <- @valid_uri_schemes do
+    defp invalid_destination?(unquote(scheme) <> _), do: false
+  end
+  defp invalid_destination?({scheme, _}) when is_atom(scheme) do
+    false
+  end
+  defp invalid_destination?(to) when is_binary(to) do
+    String.contains?(to, ":")
+  end
+  defp invalid_destination?(_), do: true
 end


Test patch:
diff --git a/test/phoenix_html/link_test.exs b/test/phoenix_html/link_test.exs
index 26ef88b..bf14cf8 100644
--- a/test/phoenix_html/link_test.exs
+++ b/test/phoenix_html/link_test.exs
@@ -31,6 +31,17 @@ defmodule Phoenix.HTML.LinkTest do
     assert safe_to_string(link(to: "/hello", do: "world")) == ~s[<a href="/hello">world</a>]
   end
 
+  test "link with scheme tuple" do
+    assert safe_to_string(link("foo", to: {:javascript, "alert(1)"})) ==
+           ~s[<a href="javascript:alert(1)">foo</a>]
+
+    assert safe_to_string(link("foo", to: {:javascript, {:safe, "alert(1)"}})) ==
+           ~s[<a href="javascript:alert(1)">foo</a>]
+
+    assert safe_to_string(link("foo", to: {:safe, {:javascript, "alert(1)"}})) ==
+           ~s[<a href="javascript:alert(1)">foo</a>]
+  end
+
   test "link with invalid args" do
     msg = "expected non-nil value for :to in link/2"
     assert_raise ArgumentError, msg, fn ->
@@ -46,6 +57,14 @@ defmodule Phoenix.HTML.LinkTest do
     assert_raise ArgumentError, msg, fn ->
       link(to: "/hello-world")
     end
+
+    msg = """
+          unsupported scheme given to link/2. In case you want to link to an
+          unknown or unsafe scheme, such as javascript, use a tuple: {:javascript, rest}
+          """
+    assert_raise ArgumentError, msg, fn ->
+      link("foo", to: "javascript:alert(1)")
+    end
   end
 
   test "button with post (default)" do
@@ -84,4 +103,14 @@ defmodule Phoenix.HTML.LinkTest do
     assert safe_to_string(button("hello", to: "/world", class: "btn rounded", id: "btn")) ==
            ~s[<button class="btn rounded" data-csrf="#{csrf_token}" data-method="post" data-to="/world" id="btn">hello</button>]
   end
+
+  test "button with invalid args" do
+    msg = """
+          unsupported scheme given to button/2. In case you want to link to an
+          unknown or unsafe scheme, such as javascript, use a tuple: {:javascript, rest}
+          """
+    assert_raise ArgumentError, msg, fn ->
+      button("foo", to: "javascript:alert(1)", method: :get)
+    end
+  end
 end