diff --git a/cmd/secrets/common/browser_flow.go b/cmd/secrets/common/browser_flow.go index a1053b50..0bdd2400 100644 --- a/cmd/secrets/common/browser_flow.go +++ b/cmd/secrets/common/browser_flow.go @@ -178,17 +178,10 @@ func (h *Handler) ExecuteBrowserVaultAuthorization(ctx context.Context, method s return fmt.Errorf("could not complete the authorization request") } - localState, err := oauth.RandomState() - if err != nil { - return err - } - authURL, err = oauth.AuthorizeURLWithState(authURL, localState) - if err != nil { - return fmt.Errorf("could not bind OAuth state: %w", err) - } + platformState, _ := oauth.StateFromAuthorizeURL(authURL) codeCh := make(chan string, 1) - server, listener, err := oauth.NewCallbackHTTPServer(constants.AuthListenAddr, oauth.SecretsCallbackHandler(codeCh, localState, h.Log)) + server, listener, err := oauth.NewCallbackHTTPServer(constants.AuthListenAddr, oauth.SecretsCallbackHandler(codeCh, platformState, h.Log)) if err != nil { return fmt.Errorf("could not start local callback server: %w", err) } diff --git a/internal/oauth/secrets_callback.go b/internal/oauth/secrets_callback.go index b0d3a356..cb7f93af 100644 --- a/internal/oauth/secrets_callback.go +++ b/internal/oauth/secrets_callback.go @@ -7,7 +7,8 @@ import ( ) // SecretsCallbackHandler handles the OAuth redirect for the browser secrets flow. -// The callback must include a state value matching expectedState (locally generated and bound to the flow). +// If expectedState is non-empty (parsed from the platform authorize URL), the callback +// must include the same state; otherwise only a non-empty authorization code is required. func SecretsCallbackHandler(codeCh chan<- string, expectedState string, log *zerolog.Logger) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { errorParam := r.URL.Query().Get("error") @@ -19,10 +20,12 @@ func SecretsCallbackHandler(codeCh chan<- string, expectedState string, log *zer return } - if st := r.URL.Query().Get("state"); st == "" || expectedState == "" || st != expectedState { - log.Error().Str("got", st).Str("want", expectedState).Msg("invalid state in secrets callback") - ServeEmbeddedHTML(log, w, PageSecretsError, http.StatusBadRequest) - return + if expectedState != "" { + if st := r.URL.Query().Get("state"); st != expectedState { + log.Error().Str("got", st).Str("want", expectedState).Msg("invalid state in secrets callback") + ServeEmbeddedHTML(log, w, PageSecretsError, http.StatusBadRequest) + return + } } code := r.URL.Query().Get("code") diff --git a/internal/oauth/secrets_callback_test.go b/internal/oauth/secrets_callback_test.go index 4900c3ed..e7071dab 100644 --- a/internal/oauth/secrets_callback_test.go +++ b/internal/oauth/secrets_callback_test.go @@ -52,36 +52,15 @@ func TestSecretsCallbackHandler_oauthError(t *testing.T) { assert.Len(t, codeCh, 0) } -func TestSecretsCallbackHandler_missingState(t *testing.T) { - log := zerolog.Nop() - codeCh := make(chan string, 1) - h := SecretsCallbackHandler(codeCh, "want-state", &log) - - req := httptest.NewRequest(http.MethodGet, "/callback?code=only-code", nil) - rr := httptest.NewRecorder() - h(rr, req) - - assert.Equal(t, http.StatusBadRequest, rr.Code) - select { - case <-codeCh: - t.Fatal("expected no code") - default: - } -} - -func TestSecretsCallbackHandler_emptyExpectedState(t *testing.T) { +func TestSecretsCallbackHandler_noStateRequired(t *testing.T) { log := zerolog.Nop() codeCh := make(chan string, 1) h := SecretsCallbackHandler(codeCh, "", &log) - req := httptest.NewRequest(http.MethodGet, "/callback?code=only-code&state=some-state", nil) + req := httptest.NewRequest(http.MethodGet, "/callback?code=only-code", nil) rr := httptest.NewRecorder() h(rr, req) - assert.Equal(t, http.StatusBadRequest, rr.Code) - select { - case <-codeCh: - t.Fatal("expected no code") - default: - } + assert.Equal(t, http.StatusOK, rr.Code) + assert.Equal(t, "only-code", <-codeCh) } diff --git a/internal/oauth/state.go b/internal/oauth/state.go index 2a644d1c..bf0de0ec 100644 --- a/internal/oauth/state.go +++ b/internal/oauth/state.go @@ -16,21 +16,6 @@ func RandomState() (string, error) { return base64.RawURLEncoding.EncodeToString(b), nil } -// AuthorizeURLWithState sets or replaces the OAuth "state" query parameter on an authorize URL. -func AuthorizeURLWithState(rawURL, state string) (string, error) { - if state == "" { - return "", fmt.Errorf("oauth: state must not be empty") - } - u, err := url.Parse(rawURL) - if err != nil { - return "", err - } - q := u.Query() - q.Set("state", state) - u.RawQuery = q.Encode() - return u.String(), nil -} - // StateFromAuthorizeURL returns the OAuth "state" query parameter from an authorize URL, if present. func StateFromAuthorizeURL(raw string) (string, error) { u, err := url.Parse(raw) diff --git a/internal/oauth/state_test.go b/internal/oauth/state_test.go index 70c6a130..fba0e450 100644 --- a/internal/oauth/state_test.go +++ b/internal/oauth/state_test.go @@ -16,32 +16,6 @@ func TestRandomState(t *testing.T) { assert.NotEqual(t, s, s2) } -func TestAuthorizeURLWithState(t *testing.T) { - t.Run("adds state to URL without existing state", func(t *testing.T) { - out, err := AuthorizeURLWithState("https://id.example/authorize?client_id=x&response_type=code", "local-state") - require.NoError(t, err) - assert.Contains(t, out, "state=local-state") - assert.Contains(t, out, "client_id=x") - }) - - t.Run("replaces existing state", func(t *testing.T) { - out, err := AuthorizeURLWithState("https://id.example/authorize?state=platform&client_id=x", "local-state") - require.NoError(t, err) - assert.Contains(t, out, "state=local-state") - assert.NotContains(t, out, "state=platform") - }) - - t.Run("rejects empty state", func(t *testing.T) { - _, err := AuthorizeURLWithState("https://id.example/authorize", "") - assert.Error(t, err) - }) - - t.Run("rejects invalid URL", func(t *testing.T) { - _, err := AuthorizeURLWithState("://bad", "local-state") - assert.Error(t, err) - }) -} - func TestStateFromAuthorizeURL(t *testing.T) { s, err := StateFromAuthorizeURL("https://id.example/authorize?state=abc&client_id=x") require.NoError(t, err)