Fix double slash in redirect URL redirect location (#1515)

Closes #1514

.credo.exs

@@ -118,7 +118,7 @@
           #
           {Credo.Check.Refactor.Apply, []},
           {Credo.Check.Refactor.CondStatements, []},
-          {Credo.Check.Refactor.CyclomaticComplexity, []},
+          {Credo.Check.Refactor.CyclomaticComplexity, [max_complexity: 12]},
           {Credo.Check.Refactor.FunctionArity, []},
           {Credo.Check.Refactor.LongQuoteBlocks, []},
           {Credo.Check.Refactor.MatchInCondition, []},

apps/fz_http/lib/fz_http/auth.ex

@@ -3,8 +3,13 @@ defmodule FzHttp.Auth do
 
   def fetch_oidc_provider_config(provider_id) do
     with {:ok, provider} <- fetch_provider(:openid_connect_providers, provider_id) do
-      external_url = FzHttp.Config.fetch_env!(:fz_http, :external_url)
-      redirect_uri = provider.redirect_uri || "#{external_url}/auth/oidc/#{provider.id}/callback/"
+      redirect_uri =
+        if provider.redirect_uri do
+          provider.redirect_uri
+        else
+          external_url = FzHttp.Config.fetch_env!(:fz_http, :external_url)
+          "#{external_url}auth/oidc/#{provider.id}/callback/"
+        end
 
       {:ok,
        %{

apps/fz_http/lib/fz_http/config/definitions.ex

@@ -156,7 +156,7 @@ defmodule FzHttp.Config.Definitions do
   defconfig(:external_url, :string,
     changeset: fn changeset, key ->
       changeset
-      |> FzHttp.Validator.validate_uri(key)
+      |> FzHttp.Validator.validate_uri(key, require_trailing_slash: true)
       |> FzHttp.Validator.normalize_url(key)
     end
   )

apps/fz_http/lib/fz_http/validator.ex

@@ -18,6 +18,7 @@ defmodule FzHttp.Validator do
 
   def validate_uri(changeset, field, opts \\ []) when is_atom(field) do
     valid_schemes = Keyword.get(opts, :schemes, ~w[http https])
+    require_trailing_slash? = Keyword.get(opts, :require_trailing_slash, false)
 
     validate_change(changeset, field, fn _current_field, value ->
       case URI.new(value) do
@@ -32,6 +33,10 @@ defmodule FzHttp.Validator do
             uri.scheme not in valid_schemes ->
               [{field, "only #{Enum.join(valid_schemes, ", ")} schemes are supported"}]
 
+            require_trailing_slash? and not is_nil(uri.path) and
+                not String.ends_with?(uri.path, "/") ->
+              [{field, "does not end with a trailing slash"}]
+
             true ->
               []
           end
@@ -48,7 +53,7 @@ defmodule FzHttp.Validator do
       uri = URI.parse(value)
       scheme = uri.scheme || "https"
       port = URI.default_port(scheme)
-      path = uri.path || "/"
+      path = maybe_add_trailing_slash(uri.path || "/")
       uri = %{uri | scheme: scheme, port: port, path: path}
       uri_string = URI.to_string(uri)
       put_change(changeset, field, uri_string)
@@ -57,6 +62,14 @@ defmodule FzHttp.Validator do
     end
   end
 
+  defp maybe_add_trailing_slash(value) do
+    if String.ends_with?(value, "/") do
+      value
+    else
+      value <> "/"
+    end
+  end
+
   def validate_no_duplicates(changeset, field) when is_atom(field) do
     validate_change(changeset, field, fn _current_field, list when is_list(list) ->
       list

apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex

@@ -133,10 +133,7 @@ defmodule FzHttpWeb.SettingLive.OIDCFormComponent do
           <div class="control">
             <%= text_input(f, :redirect_uri,
               placeholder:
-                Path.join(
-                  @external_url,
-                  "auth/oidc/#{input_value(f, :id) || "{CONFIG_ID}"}/callback/"
-                ),
+                "#{@external_url}auth/oidc/#{input_value(f, :id) || "{CONFIG_ID}"}/callback/",
               class: "input #{input_error_class(f, :redirect_uri)}"
             ) %>
           </div>
@@ -147,10 +144,7 @@ defmodule FzHttpWeb.SettingLive.OIDCFormComponent do
             Optionally override the Redirect URI. Must match the redirect URI set in your IdP.
             In most cases you shouldn't change this. By default
             <code>
-              <%= Path.join(
-                @external_url,
-                "auth/oidc/#{input_value(f, :id) || "{CONFIG_ID}"}/callback/"
-              ) %>
+              <%= "#{@external_url}auth/oidc/#{input_value(f, :id) || "{CONFIG_ID}"}/callback/" %>
             </code>
             is used.
           </p>

apps/fz_http/test/fz_http/auth_test.exs

@@ -36,7 +36,7 @@ defmodule FzHttp.AuthTest do
                   client_id: attrs["client_id"],
                   client_secret: attrs["client_secret"],
                   discovery_document_uri: attrs["discovery_document_uri"],
-                  redirect_uri: "http://foo.bar.com//auth/oidc/google/callback/",
+                  redirect_uri: "http://foo.bar.com/auth/oidc/google/callback/",
                   response_type: attrs["response_type"],
                   scope: attrs["scope"]
                 }}

apps/fz_http/test/fz_http/config_test.exs

@@ -155,6 +155,34 @@ defmodule FzHttp.ConfigTest do
       eg: `https://firezone.mycorp.com/vpn`.
 
 
+      You can find more information on configuration here: https://www.firezone.dev/docs/reference/env-vars/#environment-variable-listing
+      """
+
+      assert_raise RuntimeError, message, fn ->
+        fetch_source_and_configs!([:external_url])
+      end
+    end
+
+    test "raises an error when value is invalid" do
+      put_system_env_override(:external_url, "https://example.com/vpn")
+
+      message = """
+      Invalid configuration for 'external_url' retrieved from environment variable EXTERNAL_URL.
+
+      Errors:
+
+       - `"https://example.com/vpn"`: does not end with a trailing slash
+
+      ## Documentation
+
+      The external URL the web UI will be accessible at.
+
+      Must be a valid and public FQDN for ACME SSL issuance to function.
+
+      You can add a path suffix if you want to serve firezone from a non-root path,
+      eg: `https://firezone.mycorp.com/vpn`.
+
+
       You can find more information on configuration here: https://www.firezone.dev/docs/reference/env-vars/#environment-variable-listing
       """