Close what the third CLI review found

Escape cmd.exe metacharacters before handing a server-supplied URL to the
Windows browser opener. erts quotes an argument only when it contains a
space or a quote, so a verification URI carrying a bare & reached cmd as a
command separator.

Classify a refresh response by what it says. Only 400 and 401 mean the
refresh token is dead and clear the stored credentials; every other status,
a transport failure, and a 200 whose body does not carry a usable
access_token and expires_in leave the token in place.

Renew on a token_expired challenge instead of resolving the same bearer
again, and bound the renewals. The api path resolved with renewal disabled,
so a token outside its expiry window came back unchanged and the request
retried forever. The retry budget lives with the other retry state so it
survives a reauthentication rather than resetting.

Validate the device authorization, poll and refresh responses before
destructuring them, so a malformed 200 returns an error the callers already
handle instead of a badmatch, a badarith, or a badarg out of timer:sleep.

Distinguish a missing sso_reauth_required from a malformed one. Missing
still means nothing lapsed, for servers that do not send it; a value that
is not a list of binaries no longer reads as an authoritative empty set
that clears the stored organizations.

Hold the device authentication lock over credential acquisition only.
Taking a lock the same process already holds adds no reference, and
releasing the inner one deleted the entry, so the outer body ran unlocked
and a second process could start a concurrent device authentication.

Author: ericmj

src/hex_api_oauth.erl

@@ -12,7 +12,8 @@
     sso_reauth_required/1,
     revoke_token/3,
     client_credentials_token/4,
-    client_credentials_token/5
+    client_credentials_token/5,
+    win_cmd_args/1
 ]).
 
 -export_type([oauth_tokens/0, device_auth_error/0]).
@@ -146,28 +147,47 @@ device_auth_flow(Config, ClientId, Scope, PromptUser) ->
 ) -> {ok, oauth_tokens()} | {error, device_auth_error()}.
 device_auth_flow(Config, ClientId, Scope, PromptUser, Opts) ->
     case device_authorization(Config, ClientId, Scope, Opts) of
-        {ok, {200, _, DeviceResponse}} when is_map(DeviceResponse) ->
-            #{
-                <<"device_code">> := DeviceCode,
-                <<"user_code">> := UserCode,
-                <<"verification_uri_complete">> := VerificationUri,
-                <<"expires_in">> := ExpiresIn,
-                <<"interval">> := IntervalSeconds
-            } = DeviceResponse,
-            ok = PromptUser(VerificationUri, UserCode),
-            OpenBrowser = proplists:get_value(open_browser, Opts, false),
-            case OpenBrowser of
-                true -> open_browser(VerificationUri);
-                false -> ok
-            end,
-            ExpiresAt = erlang:system_time(second) + ExpiresIn,
-            poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt);
+        {ok, {200, _, DeviceResponse}} ->
+            case device_authorization_fields(DeviceResponse) of
+                {ok, DeviceCode, UserCode, VerificationUri, ExpiresIn, IntervalSeconds} ->
+                    ok = PromptUser(VerificationUri, UserCode),
+                    OpenBrowser = proplists:get_value(open_browser, Opts, false),
+                    case OpenBrowser of
+                        true -> open_browser(VerificationUri);
+                        false -> ok
+                    end,
+                    ExpiresAt = erlang:system_time(second) + ExpiresIn,
+                    poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt);
+                error ->
+                    {error, {device_auth_failed, 200, DeviceResponse}}
+            end;
         {ok, {Status, _, Body}} ->
             {error, {device_auth_failed, Status, Body}};
         {error, Reason} ->
             {error, Reason}
     end.
 
+%% @private
+%% The fields the flow goes on to use, in the types it uses them as: the
+%% interval is slept on and the expiry is added to a timestamp.
+device_authorization_fields(#{
+    <<"device_code">> := DeviceCode,
+    <<"user_code">> := UserCode,
+    <<"verification_uri_complete">> := VerificationUri,
+    <<"expires_in">> := ExpiresIn,
+    <<"interval">> := Interval
+}) when
+    is_binary(DeviceCode),
+    is_binary(UserCode),
+    is_binary(VerificationUri),
+    is_integer(ExpiresIn),
+    is_integer(Interval),
+    Interval >= 0
+->
+    {ok, DeviceCode, UserCode, VerificationUri, ExpiresIn, Interval};
+device_authorization_fields(_DeviceResponse) ->
+    error.
+
 %% @private
 poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) ->
     Now = erlang:system_time(second),
@@ -177,17 +197,20 @@ poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt) ->
         false ->
             timer:sleep(IntervalSeconds * 1000),
             case poll_device_token(Config, ClientId, DeviceCode) of
-                {ok, {200, _, TokenResponse}} when is_map(TokenResponse) ->
-                    #{
-                        <<"access_token">> := AccessToken,
-                        <<"expires_in">> := ExpiresIn
-                    } = TokenResponse,
-                    Tokens = #{
-                        access_token => AccessToken,
-                        expires_at => erlang:system_time(second) + ExpiresIn,
-                        sso_reauth_required => sso_reauth_required(TokenResponse)
-                    },
-                    {ok, put_refresh_token(Tokens, TokenResponse)};
+                {ok, {200, _, TokenResponse}} ->
+                    case token_response_fields(TokenResponse) of
+                        {ok, AccessToken, ExpiresIn} ->
+                            Tokens = #{
+                                access_token => AccessToken,
+                                expires_at => erlang:system_time(second) + ExpiresIn
+                            },
+                            {ok,
+                                put_sso_reauth_required(
+                                    put_refresh_token(Tokens, TokenResponse), TokenResponse
+                                )};
+                        error ->
+                            {error, {poll_failed, 200, TokenResponse}}
+                    end;
                 {ok, {400, _, #{<<"error">> := <<"authorization_pending">>}}} ->
                     poll_for_token_loop(Config, ClientId, DeviceCode, IntervalSeconds, ExpiresAt);
                 {ok, {400, _, #{<<"error">> := <<"slow_down">>}}} ->
@@ -374,16 +397,25 @@ revoke_token(Config, ClientId, Token) ->
     },
     hex_api:post(Config, Path, Params).
 
-%% @private
+%% @doc
 %% Organizations a token response says the session has to authenticate against
-%% their identity provider for. Older servers do not send the field at all,
-%% which means nothing is lapsed.
--spec sso_reauth_required(map()) -> [binary()].
-sso_reauth_required(TokenResponse) ->
-    case maps:get(<<"sso_reauth_required">>, TokenResponse, []) of
-        Organizations when is_list(Organizations) -> Organizations;
-        _Other -> []
-    end.
+%% their identity provider for.
+%%
+%% Returns `{ok, []}' when the response does not carry the field, which is what
+%% servers that predate it send and means nothing is lapsed. Returns `error'
+%% when the field is there in a shape that cannot be read, which says nothing
+%% about what has lapsed and must not be taken for the empty set.
+%% @end
+-spec sso_reauth_required(map()) -> {ok, [binary()]} | error.
+sso_reauth_required(#{<<"sso_reauth_required">> := Organizations}) when is_list(Organizations) ->
+    case lists:all(fun is_binary/1, Organizations) of
+        true -> {ok, Organizations};
+        false -> error
+    end;
+sso_reauth_required(#{<<"sso_reauth_required">> := _Organizations}) ->
+    error;
+sso_reauth_required(_TokenResponse) ->
+    {ok, []}.
 
 %%====================================================================
 %% Internal functions
@@ -413,7 +445,7 @@ spawn_browser(Url) ->
             {unix, _} ->
                 {"xdg-open", [Url]};
             {win32, _} ->
-                {"cmd", ["/c", "start", "", Url]}
+                {"cmd", win_cmd_args(Url)}
         end,
     case os:find_executable(Cmd) of
         false ->
@@ -423,6 +455,37 @@ spawn_browser(Url) ->
             ok
     end.
 
+%% @private
+%% `start' takes its first quoted argument as the window title, so the empty
+%% string keeps the URL in the position `start' opens.
+-spec win_cmd_args(string()) -> [string()].
+win_cmd_args(Url) ->
+    ["/c", "start", "", escape_win_cmd(Url)].
+
+%% @private
+%% cmd.exe parses the command line before `start' sees it, and erts only quotes
+%% an argument that contains whitespace, so every character cmd acts on is
+%% prefixed with a caret. `%' is included because cmd expands `%NAME%' before it
+%% scans for separators.
+escape_win_cmd(Url) ->
+    lists:flatmap(fun escape_win_cmd_character/1, Url).
+
+%% @private
+escape_win_cmd_character(Character) when
+    Character =:= $^;
+    Character =:= $&;
+    Character =:= $|;
+    Character =:= $<;
+    Character =:= $>;
+    Character =:= $(;
+    Character =:= $);
+    Character =:= $";
+    Character =:= $%
+->
+    [$^, Character];
+escape_win_cmd_character(Character) ->
+    [Character].
+
 %% @private
 %% Whether a URL uses the http:// or https:// scheme.
 -spec valid_http_url(binary()) -> boolean().
@@ -433,6 +496,15 @@ valid_http_url(Url) when is_binary(Url) ->
         _ -> false
     end.
 
+%% @private
+%% The access token and its lifetime, in the types the caller uses them as.
+token_response_fields(#{<<"access_token">> := AccessToken, <<"expires_in">> := ExpiresIn}) when
+    is_binary(AccessToken), is_integer(ExpiresIn)
+->
+    {ok, AccessToken, ExpiresIn};
+token_response_fields(_TokenResponse) ->
+    error.
+
 %% @private
 %% A response without a refresh token carries no key at all rather than a
 %% placeholder, so what a build tool stores is only ever a real token.
@@ -441,6 +513,15 @@ put_refresh_token(Tokens, #{<<"refresh_token">> := RefreshToken}) when is_binary
 put_refresh_token(Tokens, _TokenResponse) ->
     Tokens.
 
+%% @private
+%% A response whose sso_reauth_required cannot be read carries no key, so the
+%% caller is not handed the empty set as if the server had sent it.
+put_sso_reauth_required(Tokens, TokenResponse) ->
+    case sso_reauth_required(TokenResponse) of
+        {ok, Organizations} -> Tokens#{sso_reauth_required => Organizations};
+        error -> Tokens
+    end.
+
 %% @private
 %% Get the hostname of the current machine.
 -spec get_hostname() -> binary().