From 15bb273eb33be788a0f801636fd0185924a084dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 18 Oct 2026 20:13:32 +0200 Subject: [PATCH 1/5] Fall back when the server rejects a callback repo_key with_repo/3 answers a token_expired 401 by resolving repository auth again with Renew = {rejected, RepoKey}. A repo_key from get_auth_config passed through as step 1 both times, so renew_repo_auth_and_retry/4 got the same credential back and returned the 401, even when a global OAuth token or Workload Identity could authenticate the request. The resolution now drops a repo_key equal to the rejected one and continues as if get_auth_config hadn't returned it: other credentials in the same auth config, the parent repository, the global OAuth token, then Workload Identity. Renew becomes {skipped, RepoKey} for the rest of the resolution. That skips the same repo_key when the build tool returns it for the parent repository as well, and keeps the credentials after it from being renewed. They weren't rejected, and forcing a refresh of a session token that has no refresh token clears it. The skip runs in both the unlocked and the locked pass, so only exchanges take the repo lock. A repo_key in the config is still passed through. The caller chose it for that request, so its 401 is returned to the caller. --- CHANGELOG.md | 4 + src/hex_cli_auth.erl | 46 ++++++++-- test/hex_cli_auth_SUITE.erl | 174 +++++++++++++++++++++++++++++++++++- 3 files changed, 214 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c603b18..d6b3be3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # CHANGELOG +## Unreleased + +* When the server answers the `repo_key` from the `get_auth_config` callback with a `token_expired` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. + ## v0.20.1 (2026-10-14) * Fix `hex_tarball:unpack/2,3` and `hex_tarball:unpack_docs/2,3` looping forever when unpacking to disk a tarball with a GNU sparse member that ends in a hole, and failing on a sparse hole that crosses a 64 KiB boundary in the file, by updating the vendored `erl_tar` to OTP 29.1.2 ([GHSA-phm5-m6cj-5382](https://github.com/erlang/otp/security/advisories/GHSA-phm5-m6cj-5382), CVE-2026-96596). diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index ec9f28d..0583f5e 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -370,7 +370,10 @@ with_repo(BaseConfig, Fun) -> %% %% A resolved token the server answers with a `token_expired' 401 is renewed at %% its source (a per-repo or Workload Identity token is exchanged again, the -%% global token is refreshed) and the request is run once more. +%% global token is refreshed) and the request is run once more. A `repo_key' +%% from `get_auth_config' can't be renewed, so the request is run once more +%% with the credentials that come after it in this order, when there are any. +%% A `repo_key' in config is the caller's choice and its 401 is returned. %% %% The repository name is taken from the config (`repo_name' or `repo_organization'). %% @@ -731,7 +734,7 @@ resolve_api_auth(_Permission, Config) -> %% @private %% Resolve repo auth credentials in this order: %% 0. repo_key in config => passthrough -%% 1. repo_key from get_auth_config => passthrough +%% 1. repo_key from get_auth_config, unless the server rejected it => passthrough %% 2. trusted + auth_key + oauth_exchange => exchange for OAuth token %% 3. trusted + auth_key => use directly %% 4. trusted Hex.pm or child repository + global OAuth tokens => use those @@ -745,9 +748,12 @@ resolve_repo_auth(Config) -> %% @private %% Renew is `{rejected, RepoKey}' when the server rejected the credential we %% have, so a stored token that has not run out of time is exchanged or -%% refreshed anyway, and `false' otherwise. +%% refreshed anyway, and `false' otherwise. A rejected `repo_key' from +%% `get_auth_config' can't be renewed, so it's skipped instead, see +%% skip_rejected_repo_key/2. resolve_repo_auth(#{repo_key := RepoKey}, _Renew) when is_binary(RepoKey) -> - %% repo_key already in config, pass through directly + %% repo_key already in config, pass through directly. The caller chose it + %% for this request, so a rejection is the caller's to handle. {ok, RepoKey, #{has_refresh_token => false}}; resolve_repo_auth(Config, Renew) -> RepoName = repo_name(Config), @@ -769,12 +775,15 @@ resolve_repo_auth(Config, Renew) -> Result end. -do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew, Lock) -> +do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew0, Lock) -> Trusted = maps:get(trusted, Config, false), OAuthExchange = maps:get(oauth_exchange, Config, false), - case call_callback(Config, get_auth_config, [LookupRepo]) of + {AuthConfig, Renew} = skip_rejected_repo_key( + call_callback(Config, get_auth_config, [LookupRepo]), Renew0 + ), + case AuthConfig of #{repo_key := RepoKey} when is_binary(RepoKey) -> - %% 1. repo_key from get_auth_config => passthrough + %% 1. repo_key from get_auth_config, unless the server rejected it => passthrough {ok, RepoKey, #{has_refresh_token => false}}; #{oauth_token := OAuthToken, auth_key := AuthKey} when is_binary(AuthKey) and OAuthExchange, Trusted @@ -810,6 +819,25 @@ do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew, Lock) -> no_auth end. +%% @private +%% A repo_key the server rejected is skipped, and the resolution continues as +%% if the build tool hadn't returned it. Renew becomes `{skipped, RepoKey}', so +%% the same repo_key is skipped for the parent repository too, and the +%% credentials resolved after it, which the server didn't reject, aren't +%% renewed. +skip_rejected_repo_key(#{repo_key := RepoKey} = AuthConfig, Renew) when + Renew =:= {rejected, RepoKey}; Renew =:= {skipped, RepoKey} +-> + {maps:remove(repo_key, AuthConfig), {skipped, RepoKey}}; +skip_rejected_repo_key(AuthConfig, Renew) -> + {AuthConfig, Renew}. + +%% @private +%% Whether a stored token that hasn't run out of time is exchanged or +%% refreshed anyway. +renew_stored_token({rejected, _RepoKey}) -> true; +renew_stored_token(_Renew) -> false. + %% @private resolve_global_oauth_for_repo(<<"hexpm">>, Config, Renew) -> resolve_global_oauth_for_repo(Config, Renew); @@ -819,7 +847,7 @@ resolve_global_oauth_for_repo(_RepoName, _Config, _Renew) -> no_auth. resolve_global_oauth_for_repo(Config, Renew) -> - case resolve_oauth_token_with_context(Config, Renew =/= false) of + case resolve_oauth_token_with_context(Config, renew_stored_token(Renew)) of {ok, Token, AuthContext} -> {ok, Token, AuthContext}; {error, no_auth} -> @@ -902,7 +930,7 @@ resolve_repo_oauth_token( Renew, Lock ) -> - case {Renew =/= false orelse is_token_expired(ExpiresAt), Lock} of + case {renew_stored_token(Renew) orelse is_token_expired(ExpiresAt), Lock} of {false, _} -> %% Token is still valid, use it BearerToken = <<"Bearer ", AccessToken/binary>>, diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 2fa335c..3d80ee0 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -89,6 +89,11 @@ all() -> with_repo_device_auth_sets_repo_key_test, with_repo_token_expired_refresh_test, with_repo_token_expired_exchange_test, + with_repo_rejected_repo_key_oauth_test, + with_repo_rejected_repo_key_no_fallback_test, + with_repo_rejected_config_repo_key_test, + with_repo_repo_key_before_oauth_test, + with_repo_replaced_repo_key_test, %% token refresh failure modes refresh_transport_error_keeps_token_test, @@ -135,7 +140,8 @@ groups() -> with_repo_workload_identity_failed_required_test, with_repo_token_expired_workload_identity_test, with_repo_token_expired_workload_identity_renewed_test, - with_repo_token_expired_workload_identity_failed_test + with_repo_token_expired_workload_identity_failed_test, + with_repo_rejected_repo_key_workload_identity_test ]} ]. @@ -1229,6 +1235,142 @@ with_repo_token_expired_exchange_test(_Config) -> end, ok. +with_repo_rejected_repo_key_oauth_test(_Config) -> + %% A repo_key from get_auth_config can't be renewed, so after the server + %% rejects it the request runs again with the session. The build tool hands + %% back the same repo_key for the parent repository, which is skipped too. + %% The session token isn't what the server rejected, so it's used without + %% a refresh, which would have replaced it with a random token. + Now = erlang:system_time(second), + OrganizationAuth = #{repo_key => <<"Bearer org_token">>}, + Config = config_with_callbacks(#{ + auth_config => #{<<"hexpm:acme">> => OrganizationAuth, <<"hexpm">> => OrganizationAuth}, + oauth_tokens => + {ok, #{ + access_token => <<"global_oauth">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }} + }), + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer org_token">> -> token_expired_response(); + RepoKey -> RepoKey + end + end, + + ?assertEqual( + <<"Bearer global_oauth">>, + hex_cli_auth:with_repo(Config#{repo_organization => <<"acme">>, trusted => true}, Fun) + ), + ok. + +with_repo_rejected_repo_key_no_fallback_test(_Config) -> + %% With nothing after the rejected repo_key, the rejection is returned + %% after one request instead of the request running again with the same + %% repo_key. + Requests = counters:new(1, []), + Config = config_with_callbacks(#{ + auth_config => #{<<"hexpm:acme">> => #{repo_key => <<"Bearer org_token">>}}, + oauth_tokens => error + }), + + Fun = fun(Cfg) -> + counters:add(Requests, 1, 1), + <<"Bearer org_token">> = maps:get(repo_key, Cfg), + token_expired_response() + end, + + ?assertEqual( + token_expired_response(), + hex_cli_auth:with_repo(Config#{repo_organization => <<"acme">>, trusted => true}, Fun) + ), + ?assertEqual(1, counters:get(Requests, 1)), + ok. + +with_repo_rejected_config_repo_key_test(_Config) -> + %% A repo_key in config is the caller's choice for this request, so its + %% rejection is returned instead of the request running with the session. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + oauth_tokens => {ok, #{access_token => <<"global_oauth">>, expires_at => Now + 3600}} + }), + + Fun = fun(Cfg) -> + <<"Bearer config_token">> = maps:get(repo_key, Cfg), + token_expired_response() + end, + + ?assertEqual( + token_expired_response(), + hex_cli_auth:with_repo( + Config#{ + repo_organization => <<"acme">>, + repo_key => <<"Bearer config_token">>, + trusted => true + }, + Fun + ) + ), + ok. + +with_repo_repo_key_before_oauth_test(_Config) -> + %% A repo_key from get_auth_config that the server accepts is used ahead + %% of the session. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + auth_config => #{<<"hexpm:acme">> => #{repo_key => <<"Bearer org_token">>}}, + oauth_tokens => {ok, #{access_token => <<"global_oauth">>, expires_at => Now + 3600}} + }), + + ?assertEqual( + <<"Bearer org_token">>, + hex_cli_auth:with_repo( + Config#{repo_organization => <<"acme">>, trusted => true}, + fun(Cfg) -> maps:get(repo_key, Cfg) end + ) + ), + ok. + +with_repo_replaced_repo_key_test(_Config) -> + %% A repo_key the build tool replaced after the server rejected the old + %% one isn't the rejected credential, so the request runs again with it + %% rather than with the session. + Now = erlang:system_time(second), + Reads = counters:new(1, []), + Config = config_with_callbacks(#{ + oauth_tokens => {ok, #{access_token => <<"global_oauth">>, expires_at => Now + 3600}} + }), + Callbacks = maps:get(cli_auth_callbacks, Config), + GetAuthConfig = fun(<<"hexpm:acme">>) -> + counters:add(Reads, 1, 1), + case counters:get(Reads, 1) of + 1 -> #{repo_key => <<"Bearer old_org_token">>}; + _ -> #{repo_key => <<"Bearer new_org_token">>} + end + end, + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer old_org_token">> -> token_expired_response(); + RepoKey -> RepoKey + end + end, + + ?assertEqual( + <<"Bearer new_org_token">>, + hex_cli_auth:with_repo( + Config#{ + cli_auth_callbacks => Callbacks#{get_auth_config => GetAuthConfig}, + repo_organization => <<"acme">>, + trusted => true + }, + Fun + ) + ), + ok. + refresh_transport_error_keeps_token_test(_Config) -> %% A refresh that never reached the server says nothing about the stored %% token, so it is kept and the caller is told the difference. @@ -2192,6 +2334,36 @@ with_repo_token_expired_workload_identity_failed_test(_Config) -> ), ok. +with_repo_rejected_repo_key_workload_identity_test(_Config) -> + %% Without a session, a rejected repo_key from get_auth_config falls back + %% to the CI job's workload identity. The exchange resolves again under the + %% repo lock, which skips the rejected repo_key too. + put_github_oidc_env("https://ci.test/token"), + Config = workload_identity_config(self(), error), + Callbacks = maps:get(cli_auth_callbacks, Config), + GetAuthConfig = fun(_RepoName) -> #{repo_key => <<"Bearer org_token">>} end, + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer org_token">> -> token_expired_response(); + RepoKey -> RepoKey + end + end, + + ?assertEqual( + <<"Bearer minted.repository:acme">>, + hex_cli_auth:with_repo( + Config#{cli_auth_callbacks => Callbacks#{get_auth_config => GetAuthConfig}}, Fun + ) + ), + + receive + {workload_identity_persisted, <<"hexpm:acme">>, {ok, _Token}} -> ok + after 100 -> + error(token_not_persisted) + end, + ok. + %%==================================================================== %% Helper Functions %%==================================================================== From a1560d8017ac8494d15a29bf4c73d71342b8013f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 18 Oct 2026 22:08:15 +0200 Subject: [PATCH 2/5] Renew credentials on an invalid_token challenge hex_cli_auth renewed a credential on a 401 carrying error="token_expired", which no Hex.pm service sends. It now renews on the RFC 6750 challenge for an access token that's expired, revoked or doesn't verify, `Bearer realm="hex", error="invalid_token"`, which hexpm sends for API requests and repo.hex.pm for organization repositories. This covers API and repository requests, including the fallback from a rejected repo_key. --- CHANGELOG.md | 3 +- src/hex_cli_auth.erl | 25 ++++---- test/hex_cli_auth_SUITE.erl | 111 ++++++++++++++++++------------------ 3 files changed, 71 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6b3be3..229f87e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,8 @@ ## Unreleased -* When the server answers the `repo_key` from the `get_auth_config` callback with a `token_expired` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. +* Renew a credential and run the request again when the server answers with a 401 carrying `WWW-Authenticate: Bearer realm="hex", error="invalid_token"`, the RFC 6750 challenge for an access token that has expired, been revoked or doesn't verify. The `error="token_expired"` challenge is no longer recognized. +* When the server answers the `repo_key` from the `get_auth_config` callback with an `invalid_token` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. ## v0.20.1 (2026-10-14) diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 0583f5e..9eb359c 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -145,8 +145,8 @@ %% Maximum OTP retry attempts -define(MAX_OTP_RETRIES, 3). -%% Maximum times a 401 that says the token expired is answered by renewing the -%% credential and running the request again. +%% Maximum times a 401 that says the token is invalid is answered by renewing +%% the credential and running the request again. -define(MAX_TOKEN_RETRIES, 1). %% Both ways a refresh can leave us without a usable token: the server refused @@ -368,8 +368,9 @@ with_repo(BaseConfig, Fun) -> %%
  • Prompt via `should_authenticate' when `auth_inline' is true
  • %% %% -%% A resolved token the server answers with a `token_expired' 401 is renewed at -%% its source (a per-repo or Workload Identity token is exchanged again, the +%% A resolved token the server answers with a 401 carrying the RFC 6750 +%% challenge `Bearer realm="hex", error="invalid_token"' is renewed at its +%% source (a per-repo or Workload Identity token is exchanged again, the %% global token is refreshed) and the request is run once more. A `repo_key' %% from `get_auth_config' can't be renewed, so the request is run once more %% with the credentials that come after it in this order, when there are any. @@ -557,7 +558,7 @@ execute_repo_with_retry(BaseConfig, Fun, RepoKey) -> case Fun(BaseConfig#{repo_key => RepoKey}) of {ok, {401, Headers, _Body}} = Response -> case detect_auth_error(Headers) of - token_expired -> + invalid_token -> renew_repo_auth_and_retry(BaseConfig, Fun, RepoKey, Response); _Other -> Response @@ -1159,7 +1160,7 @@ execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> _ -> <<"Enter OTP code:">> end, handle_otp_retry(Config, Fun, AuthContext, Retries, Msg, Opts); - token_expired -> + invalid_token -> handle_token_refresh_retry(Config, Fun, AuthContext, Retries, Response, Opts); none -> Response @@ -1184,9 +1185,9 @@ handle_otp_retry(Config, Fun, AuthContext, #{otp := OtpRetries} = Retries, Messa end. %% @private -%% A 401 that says the token expired is answered by renewing the credential at -%% its source and running the request once more, the way a repository request -%% is. The renewal is counted, so a server that answers token_expired to every +%% A 401 that says the token is invalid is answered by renewing the credential +%% at its source and running the request once more, the way a repository request +%% is. The renewal is counted, so a server that answers invalid_token to every %% bearer we send it gets a bounded number of requests rather than a loop. handle_token_refresh_retry( _Config, _Fun, _AuthContext, #{token := TokenRetries}, Response, _Opts @@ -1225,7 +1226,7 @@ maybe_reauthenticate(Config, Fun, Retries, Opts) -> end. %% @private --spec detect_auth_error(hex_http:headers()) -> otp_required | invalid_totp | token_expired | none. +-spec detect_auth_error(hex_http:headers()) -> otp_required | invalid_totp | invalid_token | none. detect_auth_error(Headers) -> case maps:get(<<"www-authenticate">>, Headers, undefined) of undefined -> @@ -1241,8 +1242,8 @@ parse_www_authenticate(Value) when is_binary(Value) -> otp_required; <<"Bearer realm=\"hex\", error=\"invalid_totp\"", _/binary>> -> invalid_totp; - <<"Bearer realm=\"hex\", error=\"token_expired\"", _/binary>> -> - token_expired; + <<"Bearer realm=\"hex\", error=\"invalid_token\"", _/binary>> -> + invalid_token; _ -> none end. diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 3d80ee0..a9bc003 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -64,15 +64,15 @@ all() -> refresh_tokens_without_credentials_test, %% with_api tests - token refresh on 401 - with_api_token_expired_refresh_test, - with_api_token_expired_renews_test, - with_api_token_expired_retry_bounded_test, - with_api_token_expired_reauth_retry_bounded_test, + with_api_invalid_token_refresh_test, + with_api_invalid_token_renews_test, + with_api_invalid_token_retry_bounded_test, + with_api_invalid_token_reauth_retry_bounded_test, %% with_api tests - reauthentication after refresh failure - with_api_token_expired_reauth_yes_test, - with_api_token_expired_reauth_no_test, - with_api_token_expired_reauth_inline_false_test, + with_api_invalid_token_reauth_yes_test, + with_api_invalid_token_reauth_no_test, + with_api_invalid_token_reauth_inline_false_test, %% with_api tests - wrapper behavior with_api_optional_test, @@ -87,8 +87,8 @@ all() -> with_repo_optional_token_refresh_failed_test, with_repo_optional_401_does_not_prompt_test, with_repo_device_auth_sets_repo_key_test, - with_repo_token_expired_refresh_test, - with_repo_token_expired_exchange_test, + with_repo_invalid_token_refresh_test, + with_repo_invalid_token_exchange_test, with_repo_rejected_repo_key_oauth_test, with_repo_rejected_repo_key_no_fallback_test, with_repo_rejected_config_repo_key_test, @@ -138,9 +138,9 @@ groups() -> with_repo_workload_identity_failed_test, with_repo_workload_identity_failed_unauthenticated_test, with_repo_workload_identity_failed_required_test, - with_repo_token_expired_workload_identity_test, - with_repo_token_expired_workload_identity_renewed_test, - with_repo_token_expired_workload_identity_failed_test, + with_repo_invalid_token_workload_identity_test, + with_repo_invalid_token_workload_identity_renewed_test, + with_repo_invalid_token_workload_identity_failed_test, with_repo_rejected_repo_key_workload_identity_test ]} ]. @@ -625,8 +625,8 @@ with_api_otp_max_retries_test(_Config) -> %% Test Cases - with_api token refresh on 401 %%==================================================================== -with_api_token_expired_refresh_test(_Config) -> - %% Test token refresh when server returns token_expired +with_api_invalid_token_refresh_test(_Config) -> + %% Test token refresh when server returns invalid_token Now = erlang:system_time(second), Self = self(), Config = config_with_callbacks(#{ @@ -660,7 +660,7 @@ with_api_token_expired_refresh_test(_Config) -> {401, #{ <<"www-authenticate">> => - <<"Bearer realm=\"hex\", error=\"token_expired\"">> + <<"Bearer realm=\"hex\", error=\"invalid_token\"">> }, <<>>}}; _ -> @@ -681,8 +681,8 @@ with_api_token_expired_refresh_test(_Config) -> end, ok. -with_api_token_expired_renews_test(_Config) -> - %% An API token the server rejects as expired is refreshed even though its +with_api_invalid_token_renews_test(_Config) -> + %% An API token the server rejects as invalid is refreshed even though its %% stored expiry has not passed, the way a repository token is, and the %% request runs again with the token that came back. Now = erlang:system_time(second), @@ -699,7 +699,7 @@ with_api_token_expired_renews_test(_Config) -> Fun = fun(Cfg) -> case maps:get(api_key, Cfg) of - <<"Bearer stale_token">> -> token_expired_response(); + <<"Bearer stale_token">> -> invalid_token_response(); ApiKey -> {ok, {200, #{}, ApiKey}} end end, @@ -710,8 +710,8 @@ with_api_token_expired_renews_test(_Config) -> ), ok. -with_api_token_expired_retry_bounded_test(_Config) -> - %% A server that answers token_expired to every bearer it is sent gets a +with_api_invalid_token_retry_bounded_test(_Config) -> + %% A server that answers invalid_token to every bearer it is sent gets a %% bounded number of requests: the renewed token is tried once and the 401 %% is handed back, rather than renewed and retried without end. Now = erlang:system_time(second), @@ -728,17 +728,17 @@ with_api_token_expired_retry_bounded_test(_Config) -> CallCount = counters:new(1, []), Result = hex_cli_auth:with_api(write, Config, fun(_Cfg) -> counters:add(CallCount, 1, 1), - token_expired_response() + invalid_token_response() end), ?assertMatch({ok, {401, _Headers, _Body}}, Result), ?assertEqual(2, counters:get(CallCount, 1)), ok. -with_api_token_expired_reauth_retry_bounded_test(_Config) -> +with_api_invalid_token_reauth_retry_bounded_test(_Config) -> %% Same bound when the renewal is a device auth: the user authenticates %% once, the request runs again with the new token, and a second - %% token_expired is the caller's to handle rather than a second prompt. + %% invalid_token is the caller's to handle rather than a second prompt. Now = erlang:system_time(second), PromptCount = counters:new(1, []), CallCount = counters:new(1, []), @@ -762,7 +762,7 @@ with_api_token_expired_reauth_retry_bounded_test(_Config) -> Config, fun(_Cfg) -> counters:add(CallCount, 1, 1), - token_expired_response() + invalid_token_response() end, [{oauth_open_browser, false}] ), @@ -776,7 +776,7 @@ with_api_token_expired_reauth_retry_bounded_test(_Config) -> %% Test Cases - with_api reauthentication after refresh failure %%==================================================================== -with_api_token_expired_reauth_yes_test(_Config) -> +with_api_invalid_token_reauth_yes_test(_Config) -> %% When token refresh fails and user agrees to re-authenticate, %% device auth flow is triggered and the operation retried. Self = self(), @@ -786,7 +786,7 @@ with_api_token_expired_reauth_yes_test(_Config) -> {ok, #{ access_token => <<"initial_token">>, %% No refresh_token; token is valid so it's used, but server - %% returns token_expired 401, triggering the reauth path + %% returns invalid_token 401, triggering the reauth path expires_at => Now + 3600 }}, should_authenticate => fun(token_refresh_failed) -> @@ -825,7 +825,7 @@ with_api_token_expired_reauth_yes_test(_Config) -> {401, #{ <<"www-authenticate">> => - <<"Bearer realm=\"hex\", error=\"token_expired\"">> + <<"Bearer realm=\"hex\", error=\"invalid_token\"">> }, <<>>}}; _ -> @@ -849,7 +849,7 @@ with_api_token_expired_reauth_yes_test(_Config) -> end, ok. -with_api_token_expired_reauth_no_test(_Config) -> +with_api_invalid_token_reauth_no_test(_Config) -> %% When token refresh fails and user declines to re-authenticate, %% returns auth_declined error. Self = self(), @@ -859,7 +859,7 @@ with_api_token_expired_reauth_no_test(_Config) -> {ok, #{ access_token => <<"initial_token">>, %% No refresh_token; token is valid so it's used, but server - %% returns token_expired 401, triggering the reauth path + %% returns invalid_token 401, triggering the reauth path expires_at => Now + 3600 }}, should_authenticate => fun(token_refresh_failed) -> @@ -876,7 +876,7 @@ with_api_token_expired_reauth_no_test(_Config) -> {401, #{ <<"www-authenticate">> => - <<"Bearer realm=\"hex\", error=\"token_expired\"">> + <<"Bearer realm=\"hex\", error=\"invalid_token\"">> }, <<>>}} end @@ -890,7 +890,7 @@ with_api_token_expired_reauth_no_test(_Config) -> end, ok. -with_api_token_expired_reauth_inline_false_test(_Config) -> +with_api_invalid_token_reauth_inline_false_test(_Config) -> %% When auth_inline is false, token refresh failure returns error directly %% without prompting the user. Now = erlang:system_time(second), @@ -899,7 +899,7 @@ with_api_token_expired_reauth_inline_false_test(_Config) -> {ok, #{ access_token => <<"initial_token">>, %% No refresh_token; token is valid so it's used, but server - %% returns token_expired 401, triggering the reauth path + %% returns invalid_token 401, triggering the reauth path expires_at => Now + 3600 }}, should_authenticate => fun(_) -> error(should_not_be_called) end @@ -913,7 +913,7 @@ with_api_token_expired_reauth_inline_false_test(_Config) -> {401, #{ <<"www-authenticate">> => - <<"Bearer realm=\"hex\", error=\"token_expired\"">> + <<"Bearer realm=\"hex\", error=\"invalid_token\"">> }, <<>>}} end, @@ -1169,8 +1169,8 @@ with_repo_device_auth_sets_repo_key_test(_Config) -> ets:delete(TokenStore), ok. -with_repo_token_expired_refresh_test(_Config) -> - %% A repository token the server rejects as expired is refreshed and the +with_repo_invalid_token_refresh_test(_Config) -> + %% A repository token the server rejects as invalid is refreshed and the %% request runs again, rather than the 401 reaching the caller. Now = erlang:system_time(second), Config = config_with_callbacks(#{ @@ -1186,7 +1186,7 @@ with_repo_token_expired_refresh_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer stale_token">> -> token_expired_response(); + <<"Bearer stale_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -1195,7 +1195,7 @@ with_repo_token_expired_refresh_test(_Config) -> ?assertEqual(<<"Bearer renewed_token">>, Result), ok. -with_repo_token_expired_exchange_test(_Config) -> +with_repo_invalid_token_exchange_test(_Config) -> %% Same for a per-repo token: it is exchanged again from the auth_key it %% came from, even though its stored expiry has not passed. Now = erlang:system_time(second), @@ -1218,7 +1218,7 @@ with_repo_token_expired_exchange_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer stale_repo_token">> -> token_expired_response(); + <<"Bearer stale_repo_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -1255,7 +1255,7 @@ with_repo_rejected_repo_key_oauth_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer org_token">> -> token_expired_response(); + <<"Bearer org_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -1279,11 +1279,11 @@ with_repo_rejected_repo_key_no_fallback_test(_Config) -> Fun = fun(Cfg) -> counters:add(Requests, 1, 1), <<"Bearer org_token">> = maps:get(repo_key, Cfg), - token_expired_response() + invalid_token_response() end, ?assertEqual( - token_expired_response(), + invalid_token_response(), hex_cli_auth:with_repo(Config#{repo_organization => <<"acme">>, trusted => true}, Fun) ), ?assertEqual(1, counters:get(Requests, 1)), @@ -1299,11 +1299,11 @@ with_repo_rejected_config_repo_key_test(_Config) -> Fun = fun(Cfg) -> <<"Bearer config_token">> = maps:get(repo_key, Cfg), - token_expired_response() + invalid_token_response() end, ?assertEqual( - token_expired_response(), + invalid_token_response(), hex_cli_auth:with_repo( Config#{ repo_organization => <<"acme">>, @@ -1353,7 +1353,7 @@ with_repo_replaced_repo_key_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer old_org_token">> -> token_expired_response(); + <<"Bearer old_org_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -2269,8 +2269,8 @@ with_repo_workload_identity_failed_required_test(_Config) -> ), ok. -with_repo_token_expired_workload_identity_test(_Config) -> - %% A kept token the repository answers token_expired for is exchanged again +with_repo_invalid_token_workload_identity_test(_Config) -> + %% A kept token the repository answers invalid_token for is exchanged again %% even though its expiry has not passed. put_github_oidc_env("https://ci.test/token"), Now = erlang:system_time(second), @@ -2279,7 +2279,7 @@ with_repo_token_expired_workload_identity_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer stale_token">> -> token_expired_response(); + <<"Bearer stale_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -2287,7 +2287,7 @@ with_repo_token_expired_workload_identity_test(_Config) -> ?assertEqual(<<"Bearer minted.repository:acme">>, hex_cli_auth:with_repo(Config, Fun)), ok. -with_repo_token_expired_workload_identity_renewed_test(_Config) -> +with_repo_invalid_token_workload_identity_renewed_test(_Config) -> %% A token another request already renewed is used instead of exchanging %% again. No fixture answers the CI token URL, so the test crashes if an %% OIDC token is requested. @@ -2304,7 +2304,7 @@ with_repo_token_expired_workload_identity_renewed_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer stale_token">> -> token_expired_response(); + <<"Bearer stale_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -2312,7 +2312,7 @@ with_repo_token_expired_workload_identity_renewed_test(_Config) -> ?assertEqual(<<"Bearer renewed_token">>, hex_cli_auth:with_repo(Config, Fun)), ok. -with_repo_token_expired_workload_identity_failed_test(_Config) -> +with_repo_invalid_token_workload_identity_failed_test(_Config) -> %% The request needed the token it was renewing, so a refused renewal is %% returned instead of the 401 that asked for it. put_github_oidc_env("https://ci.test/token"), @@ -2323,7 +2323,7 @@ with_repo_token_expired_workload_identity_failed_test(_Config) -> Fun = fun(Cfg) -> <<"Bearer stale_token">> = maps:get(repo_key, Cfg), - token_expired_response() + invalid_token_response() end, ?assertEqual( @@ -2345,7 +2345,7 @@ with_repo_rejected_repo_key_workload_identity_test(_Config) -> Fun = fun(Cfg) -> case maps:get(repo_key, Cfg) of - <<"Bearer org_token">> -> token_expired_response(); + <<"Bearer org_token">> -> invalid_token_response(); RepoKey -> RepoKey end end, @@ -2650,9 +2650,10 @@ queue_jwt_bearer_response(Overrides) -> ok. %% @private -%% The 401 hexpm answers a request whose token it considers expired. -token_expired_response() -> - Headers = #{<<"www-authenticate">> => <<"Bearer realm=\"hex\", error=\"token_expired\"">>}, +%% The 401 Hex.pm answers a request whose access token is expired, revoked or +%% doesn't verify. +invalid_token_response() -> + Headers = #{<<"www-authenticate">> => <<"Bearer realm=\"hex\", error=\"invalid_token\"">>}, {ok, {401, Headers, <<"">>}}. %% @private From c40db43c1b1facac7ced9dcbcb69dbf630278bb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 18 Oct 2026 22:14:41 +0200 Subject: [PATCH 3/5] Don't send the api_key with OAuth grant requests hex_api sends the config's api_key as the authorization header, and hex_cli_auth renews a refused API token with the request's config, which still holds the refused token. hexpm authenticates an API request before routing it, so the refresh, or the device authorization that follows a failed one, was answered with the same 401, and refresh_or_clear/2 cleared the stored session token. Nothing reached this until a server sent the challenge hex_cli_auth renews on. The device authorization, token endpoint and OIDC audience requests in hex_api_oauth now drop api_key from the config; a grant is authenticated by the request body. organization_authorization/2 still sends it, since it's authorized by the session's access token. The test HTTP adapter refuses those three endpoints when a request carries an authorization header, as hexpm does for the credentials the tests use. --- CHANGELOG.md | 1 + src/hex_api_oauth.erl | 21 +++++++++++++++------ test/support/hex_http_test.erl | 14 ++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 229f87e..7c2a910 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +* Don't send the config's `api_key` with the device authorization, token and OIDC audience requests in `hex_api_oauth`. The server refuses a request carrying a credential it refuses, so renewing a refused API token had its refresh or device authorization refused too, and the refused refresh cleared the stored token. * Renew a credential and run the request again when the server answers with a 401 carrying `WWW-Authenticate: Bearer realm="hex", error="invalid_token"`, the RFC 6750 challenge for an access token that has expired, been revoked or doesn't verify. The `error="token_expired"` challenge is no longer recognized. * When the server answers the `repo_key` from the `get_auth_config` callback with an `invalid_token` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. diff --git a/src/hex_api_oauth.erl b/src/hex_api_oauth.erl index 08351cf..08ca1ce 100644 --- a/src/hex_api_oauth.erl +++ b/src/hex_api_oauth.erl @@ -85,7 +85,7 @@ device_authorization(Config, ClientId, Scope, Opts) -> <<"scope">> => Scope, <<"name">> => Name }, - hex_api:post(Config, Path, Params). + hex_api:post(grant_config(Config), Path, Params). %% @doc %% Runs the complete OAuth device authorization flow. @@ -266,7 +266,7 @@ poll_device_token(Config, ClientId, DeviceCode) -> <<"device_code">> => DeviceCode, <<"client_id">> => ClientId }, - hex_api:post(Config, Path, Params). + hex_api:post(grant_config(Config), Path, Params). %% @doc %% Refreshes an access token using a refresh token. @@ -292,7 +292,7 @@ refresh_token(Config, ClientId, RefreshToken) -> <<"refresh_token">> => RefreshToken, <<"client_id">> => ClientId }, - hex_api:post(Config, Path, Params). + hex_api:post(grant_config(Config), Path, Params). %% @doc %% Requests a browser URL to verify the organizations' SSO and 2FA requirements @@ -373,7 +373,7 @@ client_credentials_token(Config, ClientId, ApiKey, Scope, Opts) -> undefined -> Params0; Name -> Params0#{<<"name">> => Name} end, - hex_api:post(Config, Path, Params). + hex_api:post(grant_config(Config), Path, Params). %% @doc %% Exchanges an OIDC token from a CI provider for an access token scoped to @@ -397,7 +397,7 @@ jwt_bearer_token(Config, Assertion, Scope) -> <<"assertion">> => Assertion, <<"scope">> => Scope }, - hex_api:post(Config, Path, Params). + hex_api:post(grant_config(Config), Path, Params). %% @doc %% Fetches the audience Workload Identity OIDC tokens must be issued for. @@ -412,7 +412,7 @@ jwt_bearer_token(Config, Assertion, Scope) -> %% @end -spec oidc_audience(hex_core:config()) -> hex_api:response(). oidc_audience(Config) -> - hex_api:get(Config, <<"oidc/audience">>). + hex_api:get(grant_config(Config), <<"oidc/audience">>). %% @doc %% Revokes an OAuth token (RFC 7009). @@ -488,6 +488,15 @@ valid_requirements(_) -> %% Internal functions %%==================================================================== +%% @private +%% The device authorization and token endpoints and the OIDC audience take no +%% credential: a grant is authenticated by what's in the request body. hexpm +%% refuses a request whose credential it refuses before it looks at the +%% grant, and the `api_key' in the config is often the token being renewed, +%% so it isn't sent. +grant_config(Config) -> + maps:remove(api_key, Config). + %% @private %% Opens a URL in the default browser using the platform's opener: `open' on %% macOS, `xdg-open' on Linux, `start' on Windows. Returns diff --git a/test/support/hex_http_test.erl b/test/support/hex_http_test.erl index 2fdaae5..1f471d0 100644 --- a/test/support/hex_http_test.erl +++ b/test/support/hex_http_test.erl @@ -374,6 +374,20 @@ fixture(post, <>, _, {_, Body}) -> %% OAuth API +%% hexpm authenticates an API request before routing it, so a device or token +%% request that carries a credential it refuses gets a 401 and never reaches +%% the grant. Every credential in these tests is one it would refuse. +fixture(Method, <>, #{<<"authorization">> := _}, _) when + (Method =:= post andalso + (Path =:= <<"/oauth/device_authorization">> orelse Path =:= <<"/oauth/token">>)) orelse + (Method =:= get andalso Path =:= <<"/oidc/audience">>) +-> + Headers = maps:put( + <<"www-authenticate">>, <<"Bearer realm=\"hex\", error=\"invalid_token\"">>, api_headers() + ), + Payload = #{<<"message">> => <<"invalid or expired access token">>}, + {ok, {401, Headers, term_to_binary(Payload)}}; + fixture(post, <>, _, {_, Body}) -> DecodedBody = binary_to_term(Body), #{<<"client_id">> := _ClientId, <<"scope">> := _Scope} = DecodedBody, From 33ce45d4f52b61b5134e146f1a468517e8210d82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Sun, 18 Oct 2026 23:42:42 +0200 Subject: [PATCH 4/5] Read WWW-Authenticate the way RFC 9110 defines it hex_api and hex_cli_auth matched the header against the exact prefix `Bearer realm="hex", error="..."`, so a challenge with its parameters in another order or case, a token value, no realm, or another challenge in front of it wasn't recognized. hex_http:bearer_error/1 parses the header per RFC 9110 sections 11.2 and 11.6.1 (several challenges, a token68 or parameters after the scheme, case-insensitive scheme and parameter names, token or quoted-string values) and returns the error of the Bearer challenge. Both now use it to find invalid_token, totp_required and invalid_totp. A 403 with insufficient_scope isn't renewed: the token is valid, it doesn't grant the request. --- CHANGELOG.md | 3 +- src/hex_api.erl | 6 +- src/hex_cli_auth.erl | 29 ++----- src/hex_http.erl | 164 +++++++++++++++++++++++++++++++++++- test/hex_cli_auth_SUITE.erl | 36 ++++++++ test/hex_http_SUITE.erl | 110 ++++++++++++++++++++++++ 6 files changed, 322 insertions(+), 26 deletions(-) create mode 100644 test/hex_http_SUITE.erl diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c2a910..6761134 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,8 @@ ## Unreleased * Don't send the config's `api_key` with the device authorization, token and OIDC audience requests in `hex_api_oauth`. The server refuses a request carrying a credential it refuses, so renewing a refused API token had its refresh or device authorization refused too, and the refused refresh cleared the stored token. -* Renew a credential and run the request again when the server answers with a 401 carrying `WWW-Authenticate: Bearer realm="hex", error="invalid_token"`, the RFC 6750 challenge for an access token that has expired, been revoked or doesn't verify. The `error="token_expired"` challenge is no longer recognized. +* Renew a credential and run the request again when the server answers with a 401 whose Bearer challenge has `error="invalid_token"`, the RFC 6750 error for an access token that has expired, been revoked or doesn't verify. The `error="token_expired"` challenge is no longer recognized. +* Read the `WWW-Authenticate` header the way RFC 9110 defines it when looking for `invalid_token` and the two-factor `totp_required` and `invalid_totp` errors: the header can hold several challenges, the Bearer challenge's parameters can come in any order with any case, values can be tokens or quoted strings, and `realm` is optional. Only the exact `Bearer realm="hex", error="..."` prefix was recognized before. Add `hex_http:bearer_error/1`. * When the server answers the `repo_key` from the `get_auth_config` callback with an `invalid_token` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. ## v0.20.1 (2026-10-14) diff --git a/src/hex_api.erl b/src/hex_api.erl index 2327ee3..2c808c4 100644 --- a/src/hex_api.erl +++ b/src/hex_api.erl @@ -173,10 +173,10 @@ to_list(Str) -> unicode:characters_to_list(Str). %% TODO: not needed after exdoc is fixed %% @private detect_otp_error({ok, {401, Headers, Body}}) -> - case maps:get(<<"www-authenticate">>, Headers, nil) of - <<"Bearer realm=\"hex\", error=\"totp_required\"", _/binary>> -> + case hex_http:bearer_error(Headers) of + <<"totp_required">> -> {error, otp_required}; - <<"Bearer realm=\"hex\", error=\"invalid_totp\"", _/binary>> -> + <<"invalid_totp">> -> {error, invalid_totp}; _ -> {ok, {401, Headers, Body}} diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 9eb359c..4633374 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -368,9 +368,9 @@ with_repo(BaseConfig, Fun) -> %%
  • Prompt via `should_authenticate' when `auth_inline' is true
  • %% %% -%% A resolved token the server answers with a 401 carrying the RFC 6750 -%% challenge `Bearer realm="hex", error="invalid_token"' is renewed at its -%% source (a per-repo or Workload Identity token is exchanged again, the +%% A resolved token the server answers with a 401 carrying a Bearer +%% challenge with `error="invalid_token"' (RFC 6750 section 3.1) is renewed +%% at its source (a per-repo or Workload Identity token is exchanged again, the %% global token is refreshed) and the request is run once more. A `repo_key' %% from `get_auth_config' can't be renewed, so the request is run once more %% with the credentials that come after it in this order, when there are any. @@ -1228,24 +1228,11 @@ maybe_reauthenticate(Config, Fun, Retries, Opts) -> %% @private -spec detect_auth_error(hex_http:headers()) -> otp_required | invalid_totp | invalid_token | none. detect_auth_error(Headers) -> - case maps:get(<<"www-authenticate">>, Headers, undefined) of - undefined -> - none; - Value -> - parse_www_authenticate(Value) - end. - -%% @private -parse_www_authenticate(Value) when is_binary(Value) -> - case Value of - <<"Bearer realm=\"hex\", error=\"totp_required\"", _/binary>> -> - otp_required; - <<"Bearer realm=\"hex\", error=\"invalid_totp\"", _/binary>> -> - invalid_totp; - <<"Bearer realm=\"hex\", error=\"invalid_token\"", _/binary>> -> - invalid_token; - _ -> - none + case hex_http:bearer_error(Headers) of + <<"totp_required">> -> otp_required; + <<"invalid_totp">> -> invalid_totp; + <<"invalid_token">> -> invalid_token; + _ -> none end. %%==================================================================== diff --git a/src/hex_http.erl b/src/hex_http.erl index ed95b46..4c7184e 100644 --- a/src/hex_http.erl +++ b/src/hex_http.erl @@ -1,7 +1,7 @@ %% @doc %% HTTP contract. -module(hex_http). --export([request/5, request_to_file/6]). +-export([request/5, request_to_file/6, bearer_error/1]). -ifdef(TEST). -export([user_agent/1]). -endif. @@ -46,6 +46,27 @@ request_to_file(Config, Method, URI, Headers, Body, Filename) when Headers2 = put_new(<<"user-agent">>, user_agent(UserAgentFragment), Headers), Adapter:request_to_file(Method, URI, Headers2, Body, Filename, AdapterConfig). +%% @doc +%% The `error' parameter of the Bearer challenge in a response's +%% `www-authenticate' header (RFC 6750 section 3), or `undefined' when there +%% is none. +%% +%% The header is read the way RFC 9110 section 11.6.1 defines it: it can hold +%% several challenges, a scheme can be followed by a token68 or by +%% parameters, scheme and parameter names are case-insensitive, and a +%% parameter value is a token or a quoted string. +-spec bearer_error(headers()) -> binary() | undefined. +bearer_error(Headers) -> + case maps:get(<<"www-authenticate">>, Headers, undefined) of + Value when is_binary(Value) -> + case [Params || {<<"bearer">>, Params} <- parse_challenges(Value)] of + [Params | _] -> maps:get(<<"error">>, Params, undefined); + [] -> undefined + end; + undefined -> + undefined + end. + %% @private user_agent(UserAgentFragment) -> OTPRelease = erlang:system_info(otp_release), @@ -78,3 +99,144 @@ put_new(Key, Value, Map) -> {ok, _} -> Map; error -> maps:put(Key, Value, Map) end. + +%%==================================================================== +%% Internal functions - WWW-Authenticate +%%==================================================================== + +%% @private +%% WWW-Authenticate = #challenge +%% challenge = auth-scheme [ 1*SP ( token68 / #auth-param ) ] +%% +%% Parsing stops at the first thing that isn't part of a challenge and +%% returns the challenges read up to there. +parse_challenges(Value) -> + parse_challenges(skip_separators(Value), []). + +parse_challenges(Value, Acc) -> + case take_token(Value) of + {<<>>, _Rest} -> + lists:reverse(Acc); + {Scheme, Rest} -> + {Params, Rest1} = parse_challenge_params(Rest), + parse_challenges(skip_separators(Rest1), [{lowercase(Scheme), Params} | Acc]) + end. + +%% A scheme is followed by a space and either a token68 or parameters, or +%% by nothing. A token68 ends the challenge, so it is followed by a comma or +%% the end of the field. +parse_challenge_params(<> = Value) when C =:= $\s -> + Value1 = skip_ows(Value), + case take_token68(Value1) of + {<<_, _/binary>>, Rest} -> + case skip_ows(Rest) of + <<>> -> {#{}, <<>>}; + <<$,, _/binary>> = Rest1 -> {#{}, Rest1}; + _ -> parse_params(Value1, #{}) + end; + {<<>>, _Rest} -> + parse_params(Value1, #{}) + end; +parse_challenge_params(Value) -> + {#{}, Value}. + +%% auth-param = token BWS "=" BWS ( token / quoted-string ) +%% +%% A token that isn't followed by "=" is the scheme of the next challenge. +parse_params(Value, Acc) -> + case take_token(Value) of + {<<>>, _Rest} -> + {Acc, Value}; + {Name, Rest} -> + case skip_ows(Rest) of + <<$=, Rest1/binary>> -> + case take_param_value(skip_ows(Rest1)) of + {ok, ParamValue, Rest2} -> + Acc1 = put_new(lowercase(Name), ParamValue, Acc), + next_param(skip_ows(Rest2), Acc1); + error -> + {Acc, <<>>} + end; + _ -> + {Acc, Value} + end + end. + +next_param(<<$,, Rest/binary>>, Acc) -> + parse_params(skip_separators(Rest), Acc); +next_param(Value, Acc) -> + {Acc, Value}. + +take_param_value(<<$", Rest/binary>>) -> + take_quoted_string(Rest, <<>>); +take_param_value(Value) -> + case take_token(Value) of + {<<>>, _Rest} -> error; + {Token, Rest} -> {ok, Token, Rest} + end. + +%% quoted-string = DQUOTE *( qdtext / quoted-pair ) DQUOTE +take_quoted_string(<<$", Rest/binary>>, Acc) -> + {ok, Acc, Rest}; +take_quoted_string(<<$\\, C, Rest/binary>>, Acc) -> + take_quoted_string(Rest, <>); +take_quoted_string(<>, Acc) when C =/= $\\ -> + take_quoted_string(Rest, <>); +take_quoted_string(_Value, _Acc) -> + error. + +take_token(Value) -> + take_while(fun is_tchar/1, Value). + +%% token68 = 1*( ALPHA / DIGIT / "-" / "." / "_" / "~" / "+" / "/" ) *"=" +take_token68(Value) -> + case take_while(fun is_token68_char/1, Value) of + {<<>>, _Rest} = Empty -> + Empty; + {Token, Rest} -> + {Padding, Rest1} = take_while(fun(C) -> C =:= $= end, Rest), + {<>, Rest1} + end. + +take_while(Pred, Value) -> + take_while(Pred, Value, 0). + +take_while(Pred, Value, N) -> + case Value of + <<_:N/binary, C, _/binary>> -> + case Pred(C) of + true -> take_while(Pred, Value, N + 1); + false -> split_binary(Value, N) + end; + _ -> + split_binary(Value, N) + end. + +%% Commas and whitespace between the elements of a list, which can also be +%% empty (RFC 9110 section 5.6.1). +skip_separators(<>) when C =:= $\s; C =:= $\t; C =:= $, -> + skip_separators(Rest); +skip_separators(Value) -> + Value. + +skip_ows(<>) when C =:= $\s; C =:= $\t -> + skip_ows(Rest); +skip_ows(Value) -> + Value. + +is_tchar(C) when C >= $a, C =< $z -> true; +is_tchar(C) when C >= $A, C =< $Z -> true; +is_tchar(C) when C >= $0, C =< $9 -> true; +is_tchar(C) -> lists:member(C, "!#$%&'*+-.^_`|~"). + +is_token68_char(C) when C >= $a, C =< $z -> true; +is_token68_char(C) when C >= $A, C =< $Z -> true; +is_token68_char(C) when C >= $0, C =< $9 -> true; +is_token68_char(C) -> lists:member(C, "-._~+/"). + +%% Scheme and parameter names are tokens, which are ASCII. +lowercase(Value) -> + <<<<(ascii_lowercase(C))>> || <> <= Value>>. + +ascii_lowercase(C) when C >= $A, C =< $Z -> C + ($a - $A); +ascii_lowercase(C) -> C. diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index a9bc003..379e32a 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -92,6 +92,7 @@ all() -> with_repo_rejected_repo_key_oauth_test, with_repo_rejected_repo_key_no_fallback_test, with_repo_rejected_config_repo_key_test, + with_repo_insufficient_scope_test, with_repo_repo_key_before_oauth_test, with_repo_replaced_repo_key_test, @@ -1289,6 +1290,41 @@ with_repo_rejected_repo_key_no_fallback_test(_Config) -> ?assertEqual(1, counters:get(Requests, 1)), ok. +with_repo_insufficient_scope_test(_Config) -> + %% A 403 for a token without the scope is returned as it is: a new token + %% won't get access, so the session isn't refreshed and the request + %% doesn't run again. + Now = erlang:system_time(second), + Requests = counters:new(1, []), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"global_oauth">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }}, + persist_oauth_tokens => fun(_Scope, _Access, _Refresh, _Expires) -> + error(refreshed) + end + }), + Headers = #{ + <<"www-authenticate">> => <<"Bearer realm=\"hex\", error=\"insufficient_scope\"">> + }, + Response = {ok, {403, Headers, <<"">>}}, + + Fun = fun(Cfg) -> + counters:add(Requests, 1, 1), + <<"Bearer global_oauth">> = maps:get(repo_key, Cfg), + Response + end, + + ?assertEqual( + Response, + hex_cli_auth:with_repo(Config#{repo_organization => <<"acme">>, trusted => true}, Fun) + ), + ?assertEqual(1, counters:get(Requests, 1)), + ok. + with_repo_rejected_config_repo_key_test(_Config) -> %% A repo_key in config is the caller's choice for this request, so its %% rejection is returned instead of the request running with the session. diff --git a/test/hex_http_SUITE.erl b/test/hex_http_SUITE.erl new file mode 100644 index 0000000..5837792 --- /dev/null +++ b/test/hex_http_SUITE.erl @@ -0,0 +1,110 @@ +-module(hex_http_SUITE). + +-compile([export_all]). + +-include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). + +all() -> + [ + bearer_error_test, + bearer_error_parameters_test, + bearer_error_several_challenges_test, + bearer_error_quoted_string_test, + bearer_error_none_test + ]. + +bearer_error_test(_Config) -> + %% What hexpm and repo.hex.pm send + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Bearer realm=\"hex\", error=\"invalid_token\"">>) + ), + ?assertEqual( + <<"insufficient_scope">>, + bearer_error(<<"Bearer realm=\"hex\", error=\"insufficient_scope\"">>) + ), + ?assertEqual( + <<"invalid_token">>, + bearer_error( + <<"Bearer realm=\"hex\", error=\"invalid_token\", ", + "error_description=\"The access token expired\"">> + ) + ), + ok. + +bearer_error_parameters_test(_Config) -> + %% Scheme and parameter names are case-insensitive, parameters come in + %% any order, a value can be a token, there can be whitespace around "=" + %% and realm is optional (RFC 9110 section 11.2). + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"bearer ERROR=\"invalid_token\", Realm=\"hex\"">>) + ), + ?assertEqual(<<"invalid_token">>, bearer_error(<<"Bearer error = invalid_token">>)), + ?assertEqual(<<"invalid_token">>, bearer_error(<<"Bearer error=\"invalid_token\"">>)), + ?assertEqual( + <<"invalid_token">>, bearer_error(<<"Bearer realm=\"hex\",\terror=\"invalid_token\"">>) + ), + ok. + +bearer_error_several_challenges_test(_Config) -> + %% A field can hold several challenges, and a challenge can be a + %% token68 (RFC 9110 section 11.6.1). + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Basic realm=\"hex\", Bearer realm=\"hex\", error=\"invalid_token\"">>) + ), + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Negotiate a87421000492aa874209af8bc028==, Bearer error=\"invalid_token\"">>) + ), + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<", , Bearer error=\"invalid_token\",, realm=\"hex\"">>) + ), + ?assertEqual( + undefined, + bearer_error( + <<"Newauth realm=\"apps\", type=1, title=\"Login to \\\"apps\\\"\", ", + "Basic realm=\"simple\"">> + ) + ), + ?assertEqual( + <<"invalid_token">>, + bearer_error( + <<"Newauth realm=\"apps\", type=1, title=\"Login to \\\"apps\\\"\", ", + "Bearer realm=\"hex\", error=\"invalid_token\"">> + ) + ), + ok. + +bearer_error_quoted_string_test(_Config) -> + %% A quoted string can hold commas, escaped quotes and things that look + %% like parameters. + ?assertEqual( + <<"invalid_token">>, + bearer_error( + <<"Bearer error_description=\"say \\\"hi\\\", error=\\\"nope\\\"\", ", + "error=\"invalid_token\"">> + ) + ), + ?assertEqual(<<"a\"b">>, bearer_error(<<"Bearer error=\"a\\\"b\"">>)), + ok. + +bearer_error_none_test(_Config) -> + ?assertEqual(undefined, hex_http:bearer_error(#{})), + ?assertEqual(undefined, bearer_error(<<"Bearer realm=\"hex\"">>)), + ?assertEqual(undefined, bearer_error(<<"Basic realm=\"hex\"">>)), + ?assertEqual(undefined, bearer_error(<<"Bearer">>)), + ?assertEqual(undefined, bearer_error(<<"">>)), + ?assertEqual(undefined, bearer_error(<<"Bearer error=\"unterminated">>)), + %% A parameter read before the field turned malformed is kept + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Bearer error=\"invalid_token\", error_description=\"unterminated">>) + ), + ok. + +bearer_error(Value) -> + hex_http:bearer_error(#{<<"www-authenticate">> => Value}). From 838b5c36b9630e1f28e7492483abe8f383def2d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eric=20Meadows-J=C3=B6nsson?= Date: Mon, 19 Oct 2026 12:05:19 +0200 Subject: [PATCH 5/5] Renew only the rejected token and resolve again from the caller's config A renewal refreshed or exchanged whatever stored token it found, and after a skipped repo_key it renewed nothing, so a fallback token identical to the rejected repo_key was sent again, and requests rejected together each refreshed the session. A stored token is now renewed when it's the one the server rejected, or has expired, and used as it is otherwise, so callers that waited for the lock use the token another caller refreshed. Renew is `false` or `{rejected, Credential}` again. with_api's retries carried the rejected api_key inside the config, so re-authentication resolved straight back to it and prompted even when another request had stored a new session. The caller's config and the credential are now kept apart, and re-authentication resolves from the caller's config and compares with the rejected credential. A refresh that gets no usable answer is returned as token_refresh_unavailable instead of offering to authenticate again. hex_http:bearer_error/1 skips empty elements at the start of a challenge's parameter list and returns the error of the first Bearer challenge that has one. --- CHANGELOG.md | 2 +- src/hex_cli_auth.erl | 158 ++++++++++++++++++++---------------- src/hex_http.erl | 13 +-- test/hex_cli_auth_SUITE.erl | 121 +++++++++++++++++++++++++++ test/hex_http_SUITE.erl | 11 +++ 5 files changed, 227 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6761134..3316c5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## Unreleased * Don't send the config's `api_key` with the device authorization, token and OIDC audience requests in `hex_api_oauth`. The server refuses a request carrying a credential it refuses, so renewing a refused API token had its refresh or device authorization refused too, and the refused refresh cleared the stored token. -* Renew a credential and run the request again when the server answers with a 401 whose Bearer challenge has `error="invalid_token"`, the RFC 6750 error for an access token that has expired, been revoked or doesn't verify. The `error="token_expired"` challenge is no longer recognized. +* Renew a credential and run the request again when the server answers with a 401 whose Bearer challenge has `error="invalid_token"`, the RFC 6750 error for an access token that has expired, been revoked or doesn't verify. A stored token is only refreshed or exchanged when it's the one the server rejected, so requests rejected together share one renewal. A refresh that gets no usable answer returns `{error, {auth_error, token_refresh_unavailable}}`, and when the token can't be renewed, a session another request stored in the meantime is used before asking to authenticate again. The `error="token_expired"` challenge is no longer recognized. * Read the `WWW-Authenticate` header the way RFC 9110 defines it when looking for `invalid_token` and the two-factor `totp_required` and `invalid_totp` errors: the header can hold several challenges, the Bearer challenge's parameters can come in any order with any case, values can be tokens or quoted strings, and `realm` is optional. Only the exact `Bearer realm="hex", error="..."` prefix was recognized before. Add `hex_http:bearer_error/1`. * When the server answers the `repo_key` from the `get_auth_config` callback with an `invalid_token` 401, `hex_cli_auth:with_repo/2,3` skips that `repo_key` and runs the request again with the parent repository's credentials, the global OAuth token or Workload Identity, instead of returning the 401. The fallback credentials are resolved the way a first request resolves them, so a stored token that hasn't expired is used without a refresh. diff --git a/src/hex_cli_auth.erl b/src/hex_cli_auth.erl index 4633374..9a5f04b 100644 --- a/src/hex_cli_auth.erl +++ b/src/hex_cli_auth.erl @@ -303,15 +303,14 @@ with_api(Permission, BaseConfig, Fun, Opts) -> AuthInline = proplists:get_value(auth_inline, Opts, true), case resolve_api_auth(Permission, BaseConfig) of {ok, ApiKey, AuthContext} -> - Config = BaseConfig#{api_key => ApiKey}, - execute_with_retry(Config, Fun, AuthContext, initial_retries(), Opts); + execute_with_retry(BaseConfig, ApiKey, Fun, AuthContext, initial_retries(), Opts); {error, no_auth} when Optional =:= true -> %% Auth is optional, try without credentials first execute_optional_with_retry(api, BaseConfig, Fun, AuthInline, Opts); {error, no_auth} when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate maybe_authenticate_and_retry( - api, BaseConfig, Fun, no_credentials, initial_retries(), Opts + api, BaseConfig, undefined, Fun, no_credentials, initial_retries(), Opts ); {error, no_auth} -> %% auth_inline is false, just return error @@ -330,7 +329,7 @@ with_api(Permission, BaseConfig, Fun, Opts) -> %% is wrong. Offering a device flow that needs the same network is no %% help. maybe_authenticate_and_retry( - api, BaseConfig, Fun, token_refresh_failed, initial_retries(), Opts + api, BaseConfig, undefined, Fun, token_refresh_failed, initial_retries(), Opts ); {error, _} = Error -> Error @@ -410,7 +409,7 @@ with_repo(BaseConfig, Fun, Opts) -> no_auth when AuthInline =:= true -> %% No auth found, ask user if they want to authenticate maybe_authenticate_and_retry( - repo, BaseConfig, Fun, no_credentials, initial_retries(), Opts + repo, BaseConfig, undefined, Fun, no_credentials, initial_retries(), Opts ); no_auth -> %% auth_inline is false, return error @@ -451,6 +450,8 @@ repo_name(_) -> %% %% Kind says which credential the retried request needs: `api' takes the token %% as api_key, `repo' resolves repository auth and takes it as repo_key. +%% Rejected is the credential the server refused, or `undefined' when the +%% request had none. %% %% Acquiring the credential is serialized with a global lock so concurrent %% callers don't each trigger their own device auth flow. The first caller to @@ -460,11 +461,11 @@ repo_name(_) -> %% A 401 brings it back here, and global:trans/4 on a lock this process already %% holds does not take a second reference: the inner transaction's exit deletes %% the lock entry while the outer one is still running. -maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Retries, Opts) -> +maybe_authenticate_and_retry(Kind, BaseConfig, Rejected, Fun, Reason, Retries, Opts) -> Credential = global:trans( {{?MODULE, device_auth}, self()}, fun() -> - acquire_credential(Kind, BaseConfig, Reason, Opts) + acquire_credential(Kind, BaseConfig, Rejected, Reason, Opts) end, [node()], infinity @@ -477,22 +478,20 @@ maybe_authenticate_and_retry(Kind, BaseConfig, Fun, Reason, Retries, Opts) -> end. %% @private -%% Another caller may have authenticated while we waited for the lock. Re-resolve -%% and, if we get a token that differs from the one we arrived with (none when -%% credentials were missing; the rejected one on token_refresh_failed), reuse it -%% instead of prompting again. Otherwise proceed to prompt + device auth. -acquire_credential(api, BaseConfig, Reason, Opts) -> - CurrentApiKey = maps:get(api_key, BaseConfig, undefined), +%% Another caller may have authenticated while we waited for the lock. Resolve +%% again from the caller's config and, if that gives a credential other than the +%% one the server rejected, reuse it instead of prompting again. Otherwise +%% proceed to prompt + device auth. +acquire_credential(api, BaseConfig, Rejected, Reason, Opts) -> case resolve_api_auth(write, BaseConfig) of - {ok, ApiKey, AuthContext} when ApiKey =/= CurrentApiKey -> + {ok, ApiKey, AuthContext} when ApiKey =/= Rejected -> {ok, ApiKey, AuthContext}; _ -> prompt_and_device_auth(api, BaseConfig, Reason, Opts) end; -acquire_credential(repo, BaseConfig, Reason, Opts) -> - CurrentRepoKey = maps:get(repo_key, BaseConfig, undefined), +acquire_credential(repo, BaseConfig, Rejected, Reason, Opts) -> case resolve_repo_auth(BaseConfig) of - {ok, RepoKey, AuthContext} when is_binary(RepoKey), RepoKey =/= CurrentRepoKey -> + {ok, RepoKey, AuthContext} when is_binary(RepoKey), RepoKey =/= Rejected -> {ok, RepoKey, AuthContext}; _ -> prompt_and_device_auth(repo, BaseConfig, Reason, Opts) @@ -530,7 +529,7 @@ authenticated_credential(repo, BaseConfig, _Tokens) -> %% @private execute_authenticated(api, BaseConfig, Fun, ApiKey, AuthContext, Retries, Opts) -> - execute_with_retry(BaseConfig#{api_key => ApiKey}, Fun, AuthContext, Retries, Opts); + execute_with_retry(BaseConfig, ApiKey, Fun, AuthContext, Retries, Opts); execute_authenticated(repo, BaseConfig, Fun, RepoKey, _AuthContext, _Retries, _Opts) -> execute_repo_with_retry(BaseConfig, Fun, RepoKey). @@ -541,7 +540,7 @@ execute_optional_with_retry(Kind, BaseConfig, Fun, AuthInline, Opts) -> {ok, {401, _Headers, _Body}} when AuthInline =:= true -> %% Got 401, need auth - ask user if they want to authenticate maybe_authenticate_and_retry( - Kind, BaseConfig, Fun, no_credentials, initial_retries(), Opts + Kind, BaseConfig, undefined, Fun, no_credentials, initial_retries(), Opts ); {ok, {401, _Headers, _Body}} -> %% Got 401 but auth_inline is false, return error @@ -748,10 +747,11 @@ resolve_repo_auth(Config) -> %% @private %% Renew is `{rejected, RepoKey}' when the server rejected the credential we -%% have, so a stored token that has not run out of time is exchanged or -%% refreshed anyway, and `false' otherwise. A rejected `repo_key' from -%% `get_auth_config' can't be renewed, so it's skipped instead, see -%% skip_rejected_repo_key/2. +%% have, and `false' otherwise. A stored token that is the rejected one is +%% exchanged or refreshed even though it has not run out of time, and one that +%% isn't, because another caller already renewed it or because it comes after +%% a rejected `repo_key' from `get_auth_config', is used as it is. A rejected +%% `repo_key' can't be renewed, so it's skipped, see skip_rejected_repo_key/2. resolve_repo_auth(#{repo_key := RepoKey}, _Renew) when is_binary(RepoKey) -> %% repo_key already in config, pass through directly. The caller chose it %% for this request, so a rejection is the caller's to handle. @@ -776,11 +776,11 @@ resolve_repo_auth(Config, Renew) -> Result end. -do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew0, Lock) -> +do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew, Lock) -> Trusted = maps:get(trusted, Config, false), OAuthExchange = maps:get(oauth_exchange, Config, false), - {AuthConfig, Renew} = skip_rejected_repo_key( - call_callback(Config, get_auth_config, [LookupRepo]), Renew0 + AuthConfig = skip_rejected_repo_key( + call_callback(Config, get_auth_config, [LookupRepo]), Renew ), case AuthConfig of #{repo_key := RepoKey} when is_binary(RepoKey) -> @@ -822,22 +822,18 @@ do_resolve_repo_auth(RepoName, LookupRepo, Config, Renew0, Lock) -> %% @private %% A repo_key the server rejected is skipped, and the resolution continues as -%% if the build tool hadn't returned it. Renew becomes `{skipped, RepoKey}', so -%% the same repo_key is skipped for the parent repository too, and the -%% credentials resolved after it, which the server didn't reject, aren't -%% renewed. -skip_rejected_repo_key(#{repo_key := RepoKey} = AuthConfig, Renew) when - Renew =:= {rejected, RepoKey}; Renew =:= {skipped, RepoKey} --> - {maps:remove(repo_key, AuthConfig), {skipped, RepoKey}}; -skip_rejected_repo_key(AuthConfig, Renew) -> - {AuthConfig, Renew}. +%% if the build tool hadn't returned it, for the parent repository too. +skip_rejected_repo_key(#{repo_key := RepoKey} = AuthConfig, {rejected, RepoKey}) -> + maps:remove(repo_key, AuthConfig); +skip_rejected_repo_key(AuthConfig, _Renew) -> + AuthConfig. %% @private -%% Whether a stored token that hasn't run out of time is exchanged or -%% refreshed anyway. -renew_stored_token({rejected, _RepoKey}) -> true; -renew_stored_token(_Renew) -> false. +%% Whether the stored token is the credential the server rejected. +is_rejected({rejected, BearerToken}, #{access_token := AccessToken}) -> + BearerToken =:= <<"Bearer ", AccessToken/binary>>; +is_rejected(false, _Tokens) -> + false. %% @private resolve_global_oauth_for_repo(<<"hexpm">>, Config, Renew) -> @@ -848,7 +844,7 @@ resolve_global_oauth_for_repo(_RepoName, _Config, _Renew) -> no_auth. resolve_global_oauth_for_repo(Config, Renew) -> - case resolve_oauth_token_with_context(Config, renew_stored_token(Renew)) of + case resolve_oauth_token_with_context(Config, Renew) of {ok, Token, AuthContext} -> {ok, Token, AuthContext}; {error, no_auth} -> @@ -927,11 +923,11 @@ resolve_repo_oauth_token( RepoName, Config, AuthKey, - #{access_token := AccessToken, expires_at := ExpiresAt}, + #{access_token := AccessToken, expires_at := ExpiresAt} = OAuthToken, Renew, Lock ) -> - case {renew_stored_token(Renew) orelse is_token_expired(ExpiresAt), Lock} of + case {is_rejected(Renew, OAuthToken) orelse is_token_expired(ExpiresAt), Lock} of {false, _} -> %% Token is still valid, use it BearerToken = <<"Bearer ", AccessToken/binary>>, @@ -988,12 +984,14 @@ get_parent_repo_key(Config, RepoName, KeyType) -> %% @private %% Resolve OAuth token with global lock to prevent concurrent refresh attempts. -%% Renew refreshes a token that has not run out of time, for when the server -%% has rejected it anyway. +%% Renew is `{rejected, BearerToken}' when the server rejected a token, which +%% refreshes the stored token if it's that one, even though it has not run out +%% of time. %% %% A stored token that is still valid, or no stored token, is answered without -%% the lock. Only a refresh takes it, and the tokens are read again under the -%% lock in case another caller refreshed them while this one waited. +%% the lock. A renewal or a refresh takes it, and the tokens are read again +%% under the lock, so a caller that waited for another caller's refresh uses +%% the refreshed token instead of refreshing again. resolve_oauth_token_with_context(Config, false) -> case call_callback(Config, get_oauth_tokens, []) of {ok, #{expires_at := ExpiresAt} = Tokens} -> @@ -1004,12 +1002,12 @@ resolve_oauth_token_with_context(Config, false) -> error -> {error, no_auth} end; -resolve_oauth_token_with_context(Config, true) -> - resolve_oauth_token_with_lock(Config, true). +resolve_oauth_token_with_context(Config, {rejected, _BearerToken} = Renew) -> + resolve_oauth_token_with_lock(Config, Renew). resolve_oauth_token_with_lock(Config, Renew) -> Resolve = fun(#{expires_at := ExpiresAt} = Tokens) -> - case Renew orelse is_token_expired(ExpiresAt) of + case is_rejected(Renew, Tokens) orelse is_token_expired(ExpiresAt) of true -> refresh_or_clear(Config, Tokens); false -> bearer_token(Tokens) end @@ -1132,15 +1130,19 @@ initial_retries() -> #{otp => 0, otp_error => undefined, token => 0}. %% @private -execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> - case Fun(Config) of +%% Runs Fun with ApiKey. BaseConfig is the caller's config, kept apart from the +%% credential so a renewal resolves from what the caller passed rather than +%% from the key that was rejected. +execute_with_retry(BaseConfig, ApiKey, Fun, AuthContext, Retries, Opts) -> + case Fun(BaseConfig#{api_key => ApiKey}) of {error, otp_required} -> handle_otp_retry( - Config, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts + BaseConfig, ApiKey, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts ); {error, invalid_totp} -> handle_otp_retry( - Config, + BaseConfig, + ApiKey, Fun, AuthContext, Retries, @@ -1151,7 +1153,7 @@ execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> case detect_auth_error(Headers) of otp_required -> handle_otp_retry( - Config, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts + BaseConfig, ApiKey, Fun, AuthContext, Retries, <<"Enter OTP code:">>, Opts ); invalid_totp -> Msg = @@ -1159,9 +1161,11 @@ execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> invalid_totp -> <<"Invalid OTP code. Please try again:">>; _ -> <<"Enter OTP code:">> end, - handle_otp_retry(Config, Fun, AuthContext, Retries, Msg, Opts); + handle_otp_retry(BaseConfig, ApiKey, Fun, AuthContext, Retries, Msg, Opts); invalid_token -> - handle_token_refresh_retry(Config, Fun, AuthContext, Retries, Response, Opts); + handle_token_refresh_retry( + BaseConfig, ApiKey, Fun, AuthContext, Retries, Response, Opts + ); none -> Response end; @@ -1170,16 +1174,20 @@ execute_with_retry(Config, Fun, AuthContext, Retries, Opts) -> end. %% @private -handle_otp_retry(_Config, _Fun, _AuthContext, #{otp := OtpRetries}, _Message, _Opts) when +handle_otp_retry( + _BaseConfig, _ApiKey, _Fun, _AuthContext, #{otp := OtpRetries}, _Message, _Opts +) when OtpRetries >= ?MAX_OTP_RETRIES -> {error, {auth_error, otp_max_retries}}; -handle_otp_retry(Config, Fun, AuthContext, #{otp := OtpRetries} = Retries, Message, Opts) -> - case call_callback(Config, prompt_otp, [Message]) of +handle_otp_retry( + BaseConfig, ApiKey, Fun, AuthContext, #{otp := OtpRetries} = Retries, Message, Opts +) -> + case call_callback(BaseConfig, prompt_otp, [Message]) of {ok, OtpCode} -> - NewConfig = Config#{api_otp => OtpCode}, + NewConfig = BaseConfig#{api_otp => OtpCode}, NewRetries = Retries#{otp := OtpRetries + 1, otp_error := invalid_totp}, - execute_with_retry(NewConfig, Fun, AuthContext, NewRetries, Opts); + execute_with_retry(NewConfig, ApiKey, Fun, AuthContext, NewRetries, Opts); cancelled -> {error, {auth_error, otp_cancelled}} end. @@ -1189,38 +1197,46 @@ handle_otp_retry(Config, Fun, AuthContext, #{otp := OtpRetries} = Retries, Messa %% at its source and running the request once more, the way a repository request %% is. The renewal is counted, so a server that answers invalid_token to every %% bearer we send it gets a bounded number of requests rather than a loop. +%% +%% A refresh that got no usable answer says nothing about the token, so it is +%% returned rather than offering to authenticate again over the same network. handle_token_refresh_retry( - _Config, _Fun, _AuthContext, #{token := TokenRetries}, Response, _Opts + _BaseConfig, _ApiKey, _Fun, _AuthContext, #{token := TokenRetries}, Response, _Opts ) when TokenRetries >= ?MAX_TOKEN_RETRIES -> Response; handle_token_refresh_retry( - Config, Fun, AuthContext, #{token := TokenRetries} = Retries, _Response, Opts + BaseConfig, ApiKey, Fun, AuthContext, #{token := TokenRetries} = Retries, _Response, Opts ) -> NewRetries = Retries#{token := TokenRetries + 1}, %% Only attempt refresh if we have a refresh token case maps:get(has_refresh_token, AuthContext, false) of true -> - case resolve_oauth_token_with_context(Config, true) of + case resolve_oauth_token_with_context(BaseConfig, {rejected, ApiKey}) of {ok, NewBearerToken, NewAuthContext} -> - NewConfig = Config#{api_key => NewBearerToken}, - execute_with_retry(NewConfig, Fun, NewAuthContext, NewRetries, Opts); + execute_with_retry( + BaseConfig, NewBearerToken, Fun, NewAuthContext, NewRetries, Opts + ); + {error, {auth_error, token_refresh_unavailable}} = Error -> + Error; {error, _} -> - maybe_reauthenticate(Config, Fun, NewRetries, Opts) + maybe_reauthenticate(BaseConfig, ApiKey, Fun, NewRetries, Opts) end; false -> - maybe_reauthenticate(Config, Fun, NewRetries, Opts) + maybe_reauthenticate(BaseConfig, ApiKey, Fun, NewRetries, Opts) end. %% @private %% After token refresh failure, prompt the user to re-authenticate via device auth %% (only when auth_inline is true). Mirrors Hex.OAuth.reauthenticate/1. -maybe_reauthenticate(Config, Fun, Retries, Opts) -> +maybe_reauthenticate(BaseConfig, ApiKey, Fun, Retries, Opts) -> AuthInline = proplists:get_value(auth_inline, Opts, true), case AuthInline of true -> - maybe_authenticate_and_retry(api, Config, Fun, token_refresh_failed, Retries, Opts); + maybe_authenticate_and_retry( + api, BaseConfig, ApiKey, Fun, token_refresh_failed, Retries, Opts + ); false -> {error, {auth_error, token_refresh_failed}} end. diff --git a/src/hex_http.erl b/src/hex_http.erl index 4c7184e..64482a9 100644 --- a/src/hex_http.erl +++ b/src/hex_http.erl @@ -47,9 +47,9 @@ request_to_file(Config, Method, URI, Headers, Body, Filename) when Adapter:request_to_file(Method, URI, Headers2, Body, Filename, AdapterConfig). %% @doc -%% The `error' parameter of the Bearer challenge in a response's -%% `www-authenticate' header (RFC 6750 section 3), or `undefined' when there -%% is none. +%% The `error' parameter of the first Bearer challenge that has one in a +%% response's `www-authenticate' header (RFC 6750 section 3), or `undefined' +%% when there is none. %% %% The header is read the way RFC 9110 section 11.6.1 defines it: it can hold %% several challenges, a scheme can be followed by a token68 or by @@ -59,8 +59,8 @@ request_to_file(Config, Method, URI, Headers, Body, Filename) when bearer_error(Headers) -> case maps:get(<<"www-authenticate">>, Headers, undefined) of Value when is_binary(Value) -> - case [Params || {<<"bearer">>, Params} <- parse_challenges(Value)] of - [Params | _] -> maps:get(<<"error">>, Params, undefined); + case [Error || {<<"bearer">>, #{<<"error">> := Error}} <- parse_challenges(Value)] of + [Error | _] -> Error; [] -> undefined end; undefined -> @@ -135,7 +135,8 @@ parse_challenge_params(<> = Value) when C =:= $\s -> _ -> parse_params(Value1, #{}) end; {<<>>, _Rest} -> - parse_params(Value1, #{}) + %% The parameter list can start with empty elements + parse_params(skip_separators(Value1), #{}) end; parse_challenge_params(Value) -> {#{}, Value}. diff --git a/test/hex_cli_auth_SUITE.erl b/test/hex_cli_auth_SUITE.erl index 379e32a..fcfcff8 100644 --- a/test/hex_cli_auth_SUITE.erl +++ b/test/hex_cli_auth_SUITE.erl @@ -68,6 +68,9 @@ all() -> with_api_invalid_token_renews_test, with_api_invalid_token_retry_bounded_test, with_api_invalid_token_reauth_retry_bounded_test, + with_api_invalid_token_refreshed_elsewhere_test, + with_api_invalid_token_replaced_session_test, + with_api_invalid_token_refresh_unavailable_test, %% with_api tests - reauthentication after refresh failure with_api_invalid_token_reauth_yes_test, @@ -90,6 +93,7 @@ all() -> with_repo_invalid_token_refresh_test, with_repo_invalid_token_exchange_test, with_repo_rejected_repo_key_oauth_test, + with_repo_rejected_repo_key_same_token_test, with_repo_rejected_repo_key_no_fallback_test, with_repo_rejected_config_repo_key_test, with_repo_insufficient_scope_test, @@ -777,6 +781,94 @@ with_api_invalid_token_reauth_retry_bounded_test(_Config) -> %% Test Cases - with_api reauthentication after refresh failure %%==================================================================== +with_api_invalid_token_refreshed_elsewhere_test(_Config) -> + %% A token another request refreshed while this one waited for the lock is + %% used without refreshing it again. + Now = erlang:system_time(second), + Reads = counters:new(1, []), + Config = config_with_callbacks(#{ + get_oauth_tokens => fun() -> + counters:add(Reads, 1, 1), + AccessToken = + case counters:get(Reads, 1) of + 1 -> <<"stale_token">>; + _ -> <<"refreshed_token">> + end, + {ok, #{ + access_token => AccessToken, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }} + end, + persist_oauth_tokens => fun(_Scope, _Access, _Refresh, _Expires) -> + error(refreshed_again) + end + }), + + Fun = fun(Cfg) -> + case maps:get(api_key, Cfg) of + <<"Bearer stale_token">> -> invalid_token_response(); + ApiKey -> ApiKey + end + end, + + ?assertEqual(<<"Bearer refreshed_token">>, hex_cli_auth:with_api(read, Config, Fun)), + ok. + +with_api_invalid_token_replaced_session_test(_Config) -> + %% A session that can't be refreshed and was replaced by another request + %% is used instead of asking to authenticate again. + Now = erlang:system_time(second), + Reads = counters:new(1, []), + Config = config_with_callbacks(#{ + get_oauth_tokens => fun() -> + counters:add(Reads, 1, 1), + AccessToken = + case counters:get(Reads, 1) of + 1 -> <<"stale_token">>; + _ -> <<"replaced_token">> + end, + {ok, #{access_token => AccessToken, expires_at => Now + 3600}} + end, + should_authenticate => fun(_Reason) -> error(prompted) end + }), + + Fun = fun(Cfg) -> + case maps:get(api_key, Cfg) of + <<"Bearer stale_token">> -> invalid_token_response(); + ApiKey -> ApiKey + end + end, + + ?assertEqual(<<"Bearer replaced_token">>, hex_cli_auth:with_api(read, Config, Fun)), + ok. + +with_api_invalid_token_refresh_unavailable_test(_Config) -> + %% A refresh that got no usable answer says nothing about the token, so + %% it's returned rather than answered with an offer to authenticate again. + Now = erlang:system_time(second), + Config = config_with_callbacks(#{ + oauth_tokens => + {ok, #{ + access_token => <<"stale_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }}, + should_authenticate => fun(_Reason) -> error(prompted) end + }), + self() ! {hex_http_test, oauth_refresh_response, {ok, {503, #{}, <<"">>}}}, + + Fun = fun(Cfg) -> + <<"Bearer stale_token">> = maps:get(api_key, Cfg), + invalid_token_response() + end, + + ?assertEqual( + {error, {auth_error, token_refresh_unavailable}}, + hex_cli_auth:with_api(read, Config, Fun) + ), + ok. + with_api_invalid_token_reauth_yes_test(_Config) -> %% When token refresh fails and user agrees to re-authenticate, %% device auth flow is triggered and the operation retried. @@ -1267,6 +1359,35 @@ with_repo_rejected_repo_key_oauth_test(_Config) -> ), ok. +with_repo_rejected_repo_key_same_token_test(_Config) -> + %% A fallback credential that is the token the server rejected is renewed + %% rather than sent again. + Now = erlang:system_time(second), + OrganizationAuth = #{repo_key => <<"Bearer same_token">>}, + Config = config_with_callbacks(#{ + auth_config => #{<<"hexpm:acme">> => OrganizationAuth, <<"hexpm">> => OrganizationAuth}, + oauth_tokens => + {ok, #{ + access_token => <<"same_token">>, + refresh_token => <<"refresh_token">>, + expires_at => Now + 3600 + }} + }), + queue_refresh_response(#{<<"access_token">> => <<"renewed_token">>}), + + Fun = fun(Cfg) -> + case maps:get(repo_key, Cfg) of + <<"Bearer same_token">> -> invalid_token_response(); + RepoKey -> RepoKey + end + end, + + ?assertEqual( + <<"Bearer renewed_token">>, + hex_cli_auth:with_repo(Config#{repo_organization => <<"acme">>, trusted => true}, Fun) + ), + ok. + with_repo_rejected_repo_key_no_fallback_test(_Config) -> %% With nothing after the rejected repo_key, the rejection is returned %% after one request instead of the request running again with the same diff --git a/test/hex_http_SUITE.erl b/test/hex_http_SUITE.erl index 5837792..74b5560 100644 --- a/test/hex_http_SUITE.erl +++ b/test/hex_http_SUITE.erl @@ -63,6 +63,17 @@ bearer_error_several_challenges_test(_Config) -> <<"invalid_token">>, bearer_error(<<", , Bearer error=\"invalid_token\",, realm=\"hex\"">>) ), + ?assertEqual(<<"invalid_token">>, bearer_error(<<"Bearer , error=invalid_token">>)), + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Basic , realm=\"hex\", Bearer error=invalid_token">>) + ), + %% The first Bearer challenge with an error is the one that counts, so one + %% for another realm without an error doesn't hide it. + ?assertEqual( + <<"invalid_token">>, + bearer_error(<<"Bearer realm=\"first\", Bearer realm=\"hex\", error=invalid_token">>) + ), ?assertEqual( undefined, bearer_error(