From 4a4ab04ff6d5512a8f5c8ab12a13975a1becf884 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Tue, 3 Nov 2026 13:48:20 +0100 Subject: [PATCH 1/3] fix: sign out without re-authenticating and revoke the stored token `ory auth logout` loaded the config through GetAuthenticatedConfig, which validates the session first. With an expired or invalid session, or no session at all, it cleared the stored token and opened a browser login, then revoked the token from that fresh login. The token that was actually stored was never revoked, and with --quiet the command failed. SignOut now reads the config file directly and revokes whatever token is stored; RFC 7009 servers accept revocation of already invalid tokens. Tokens obtained without the offline_access scope have no refresh token, so the revocation request sent an empty token and the access token stayed valid. The access token is now revoked in that case. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DUBUDsgicGQri3GxrTV8VW --- cmd/cloudx/client/auth.go | 69 +++++++++++---- cmd/cloudx/client/auth_test.go | 148 +++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 16 deletions(-) create mode 100644 cmd/cloudx/client/auth_test.go diff --git a/cmd/cloudx/client/auth.go b/cmd/cloudx/client/auth.go index c51d8507..4b38756a 100644 --- a/cmd/cloudx/client/auth.go +++ b/cmd/cloudx/client/auth.go @@ -273,32 +273,69 @@ func redirectErr(w http.ResponseWriter, r *http.Request, err, desc string) { http.Redirect(w, r, location.String(), http.StatusFound) } +// SignOut revokes the stored OAuth2 token and removes it from the local +// configuration. It reads the configuration file directly instead of going +// through GetAuthenticatedConfig, because signing out must not require a valid +// session: an expired or invalid one would otherwise start a new login, and +// RFC 7009 servers accept revocation requests for tokens that are already +// invalid. func (h *CommandHelper) SignOut(ctx context.Context) error { - config, err := h.GetAuthenticatedConfig(ctx) - if err != nil { + config, err := readConfig(h.configLocation) + if stderrors.Is(err, ErrNoConfig) { + return nil + } else if err != nil { return err } - if config.AccessToken == nil { - return h.ClearConfig() + if config.AccessToken != nil { + if err := revokeToken(ctx, config.AccessToken); err != nil { + _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token, signing out locally only: %v\n", err) + } } + return h.ClearConfig() +} + +// revokeToken revokes the refresh token, which also revokes the access tokens +// issued with it. Tokens obtained without the offline_access scope have no +// refresh token, so the access token is revoked instead. +func revokeToken(ctx context.Context, token *oauth2.Token) error { + value, hint := token.RefreshToken, "refresh_token" + if value == "" { + value, hint = token.AccessToken, "access_token" + } + if value == "" { + return nil + } + client := oauth2ClientConfig() revoke, err := url.Parse(client.Endpoint.AuthURL) if err != nil { return err } revoke.Path = "/oauth2/revoke" - res, err := http.PostForm(revoke.String(), url.Values{ - "client_id": []string{client.ClientID}, - "token": []string{config.AccessToken.RefreshToken}, // this also revokes the associated access token - }) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, revoke.String(), strings.NewReader(url.Values{ + "client_id": {client.ClientID}, + "token": {value}, + "token_type_hint": {hint}, + }.Encode())) if err != nil { - _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token: %v\n", err) - } else { - defer res.Body.Close() - if res.StatusCode < 200 || res.StatusCode > 299 { - body, _ := io.ReadAll(res.Body) - _, _ = fmt.Fprintf(h.VerboseErrWriter, "failed to revoke access token: %v\n", string(body)) - } + return err } - return h.ClearConfig() + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + // Honor the same context HTTP client that the oauth2 package uses for the + // token exchange and refresh requests against this server. + httpClient := http.DefaultClient + if c, ok := ctx.Value(oauth2.HTTPClient).(*http.Client); ok && c != nil { + httpClient = c + } + res, err := httpClient.Do(req) + if err != nil { + return err + } + defer res.Body.Close() + if res.StatusCode < 200 || res.StatusCode > 299 { + body, _ := io.ReadAll(io.LimitReader(res.Body, 4096)) + return fmt.Errorf("unexpected status %d: %s", res.StatusCode, body) + } + return nil } diff --git a/cmd/cloudx/client/auth_test.go b/cmd/cloudx/client/auth_test.go new file mode 100644 index 00000000..7dcbaaa5 --- /dev/null +++ b/cmd/cloudx/client/auth_test.go @@ -0,0 +1,148 @@ +// Copyright © 2026 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package client_test + +import ( + "bytes" + "context" + "encoding/json" + "io" + "net/http" + "net/url" + "os" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + + "github.com/ory/cli/cmd/cloudx/client" + "github.com/ory/cli/cmd/cloudx/testhelpers" +) + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// revokeRecorder answers OAuth2 token revocation requests with the given +// status and fails the test on any other request, such as a token refresh. +func revokeRecorder(t *testing.T, status int) (*http.Client, *[]url.Values) { + var revoked []url.Values + return &http.Client{Transport: roundTripFunc(func(r *http.Request) (*http.Response, error) { + res := &http.Response{ + StatusCode: status, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("")), + Request: r, + } + if r.Method != http.MethodPost || r.URL.Path != "/oauth2/revoke" { + t.Errorf("unexpected request: %s %s", r.Method, r.URL) + res.StatusCode = http.StatusBadRequest + return res, nil + } + if assert.NoError(t, r.ParseForm()) { + revoked = append(revoked, r.PostForm) + } + return res, nil + })}, &revoked +} + +func TestSignOut(t *testing.T) { + expired := time.Now().Add(-time.Hour) + + writeConfig := func(t *testing.T, config any) string { + location := testhelpers.NewConfigFile(t) + raw, err := json.Marshal(config) + require.NoError(t, err) + require.NoError(t, os.WriteFile(location, raw, 0600)) + return location + } + + signOut := func(t *testing.T, location string, status int, opts ...client.CommandHelperOption) (revoked []url.Values, stderr string, err error) { + httpClient, rec := revokeRecorder(t, status) + ctx := context.WithValue(context.Background(), oauth2.HTTPClient, httpClient) + out := bytes.Buffer{} + h, err := client.NewCommandHelper(ctx, append([]client.CommandHelperOption{ + client.WithConfigLocation(location), + client.WithVerboseErrWriter(&out), + client.WithOpenBrowserHook(func(string) error { return testhelpers.ErrAuthFlowTriggered }), + }, opts...)...) + require.NoError(t, err) + err = h.SignOut(ctx) + return *rec, out.String(), err + } + + t.Run("revokes the refresh token of an expired session without signing in", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", RefreshToken: "refresh", Expiry: expired}, + }) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Equal(t, []url.Values{{ + "client_id": {"ory-cli"}, + "token": {"refresh"}, + "token_type_hint": {"refresh_token"}, + }}, revoked) + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("revokes the access token if there is no refresh token", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", Expiry: expired}, + }) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Equal(t, []url.Values{{ + "client_id": {"ory-cli"}, + "token": {"access"}, + "token_type_hint": {"access_token"}, + }}, revoked) + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("signs out locally if revocation fails", func(t *testing.T) { + location := writeConfig(t, client.Config{ + Version: client.ConfigVersion, + AccessToken: &oauth2.Token{AccessToken: "access", RefreshToken: "refresh", Expiry: time.Now().Add(time.Hour)}, + }) + + revoked, stderr, err := signOut(t, location, http.StatusServiceUnavailable) + require.NoError(t, err) + + assert.Len(t, revoked, 1) + assert.Contains(t, stderr, "failed to revoke") + assert.Nil(t, testhelpers.ReadConfig(t, location).AccessToken) + }) + + t.Run("does nothing without a config file", func(t *testing.T) { + location := testhelpers.NewConfigFile(t) + + revoked, _, err := signOut(t, location, http.StatusOK) + require.NoError(t, err) + + assert.Empty(t, revoked) + assert.NoFileExists(t, location) + }) + + t.Run("clears a legacy config", func(t *testing.T) { + location := writeConfig(t, map[string]string{"version": "v0alpha0", "session_token": "legacy"}) + + revoked, _, err := signOut(t, location, http.StatusOK, client.WithNoConfirm(true)) + require.NoError(t, err) + + assert.Empty(t, revoked) + raw, err := os.ReadFile(location) + require.NoError(t, err) + assert.NotContains(t, string(raw), "legacy") + assert.Equal(t, client.ConfigVersion, testhelpers.ReadConfig(t, location).Version) + }) +} From b7afd01c6649a94b885c42095f892cc7525a514c Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 4 Nov 2026 17:59:32 +0100 Subject: [PATCH 2/3] test: log in through the identifier-first console login Since ory/cloud#16172 the console login asks for the email on its own screen and only then for the password, so the browser login of every cloudx test package timed out waiting for the password field. The login helper now submits the identifier step when the email field is shown, and starts at the password when reconfirming a session. Kratos answers the identifier step with 400 and the flow's next step, so a 400 only counts as a refusal if the flow carries error messages. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DUBUDsgicGQri3GxrTV8VW --- cmd/cloudx/testhelpers/testhelpers.go | 88 +++++++++++++++++++-------- 1 file changed, 64 insertions(+), 24 deletions(-) diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index bc3619a0..bb0cd5f6 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -468,15 +468,15 @@ func redactInZip(path, secret string) error { return os.Rename(tmp.Name(), path) } -// submitPasswordForm submits the filled-in login form and fails immediately if -// Ory Network did not accept it. +// submitLoginStep submits the login form with the given method and fails +// immediately if Ory Network did not accept it. // -// It exists because the consent screen is the next thing the caller waits for. -// On a refusal the page never leaves the form, so an unchecked failure surfaces -// 30 seconds later as a missing `Allow` button, pointing at the consent screen -// instead of at the reason it never rendered: rejected credentials (400), 429 -// when a CI run drives more logins from one IP than Ory Network allows, or a 5xx. -func submitPasswordForm(t testing.TB, page playwright.Page) { +// It exists because every step is followed by a wait for the next screen. On a +// refusal the page never leaves the form, so an unchecked failure surfaces 30 +// seconds later as a missing field or `Allow` button, pointing at that screen +// instead of at the reason it never rendered: rejected credentials, 429 when a +// CI run drives more logins from one IP than Ory Network allows, or a 5xx. +func submitLoginStep(t testing.TB, page playwright.Page, method string) { // The login UI submits the flow with fetch, so this POST is the only request // to this path; the flow itself was created before the page rendered. isLoginSubmission := func(r playwright.Response) bool { @@ -484,22 +484,58 @@ func submitPasswordForm(t testing.TB, page playwright.Page) { } resp, err := page.ExpectResponse(isLoginSubmission, func() error { - return page.Locator(`[type="submit"][name="method"][value="password"]`).Click() + return page.Locator(fmt.Sprintf(`[type="submit"][name="method"][value=%q]`, method)).Click() }) - require.NoError(t, err, "the login form was never submitted") - - // A login that continues at the OAuth2 consent screen is answered with 422 - // browser_location_change_required, the only use of 422 in self-service - // flows. The login UI navigates away on it, which can discard the body - // before it is read, so the status alone has to decide. - if resp.Status() < http.StatusBadRequest || resp.Status() == http.StatusUnprocessableEntity { + require.NoError(t, err, "the %s login step was never submitted", method) + + switch resp.Status() { + case http.StatusUnprocessableEntity: + // A login that continues at the OAuth2 consent screen is answered with + // 422 browser_location_change_required, the only use of 422 in + // self-service flows. The login UI navigates away on it, which can + // discard the body before it is read, so the status alone has to decide. return + case http.StatusBadRequest: + // Kratos answers 400 both when it refuses a step and when the flow + // continues at its next step, as after the identifier of an + // identifier-first login. The UI renders that step in place, so the + // body is still there to tell the two apart. + if body, err := resp.Body(); err == nil && isNextLoginStep(body) { + return + } + default: + if resp.Status() < http.StatusBadRequest { + return + } } body, _ := resp.Text() require.FailNowf(t, "the login form was refused", - "POST %s\n%d %s\n%s\n\nThe consent screen never renders after this, so waiting for it would only time out.", - resp.URL(), resp.Status(), resp.StatusText(), body) + "POST %s (method %s)\n%d %s\n%s\n\nThe next login screen never renders after this, so waiting for it would only time out.", + resp.URL(), method, resp.Status(), resp.StatusText(), body) +} + +// isNextLoginStep reports whether a 400 answer to a login submission is the +// flow asking for its next step. Kratos answers a refusal with the flow too, +// but adds messages of type error to it; other errors are not a flow at all. +func isNextLoginStep(body []byte) bool { + var flow cloud.LoginFlow + if err := json.Unmarshal(body, &flow); err != nil { + return false + } + for _, m := range flow.Ui.Messages { + if m.Type == "error" { + return false + } + } + for _, n := range flow.Ui.Nodes { + for _, m := range n.Messages { + if m.Type == "error" { + return false + } + } + } + return true } func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, email, password string) func(uri string) error { @@ -521,19 +557,23 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai _, err := page.Goto(uri) require.NoError(t, err) - // The password field renders on a first login and when reconfirming an - // existing session, but only the first login asks for the identifier. - passwordInput := page.Locator(`[data-testid="ory/form/node/input/password"]`) - require.NoError(t, passwordInput.WaitFor()) - identifierInput := page.Locator(`[data-testid="ory/form/node/input/identifier"]`) + // Console login is identifier-first: a first login asks for the email on + // its own screen and only then for the password, while reconfirming an + // existing session starts at the password. The email stays in the form + // as a hidden field after its step, so only visible fields count. + identifierInput := page.Locator(`[data-testid="ory/form/node/input/identifier"]:visible`) + passwordInput := page.Locator(`[data-testid="ory/form/node/input/password"]:visible`) + require.NoError(t, identifierInput.Or(passwordInput).First().WaitFor()) if visible, err := identifierInput.IsVisible(); err == nil && visible { t.Logf("logging in") require.NoError(t, identifierInput.Fill(email)) + submitLoginStep(t, page, "identifier_first") + require.NoError(t, passwordInput.WaitFor()) } else { t.Logf("reconfirming password") } require.NoError(t, passwordInput.Fill(password)) - submitPasswordForm(t, page) + submitLoginStep(t, page, "password") // we wait here for the button +1s because there is some console bug that can lead to form submissions before the form action is correctly set if err := page.Locator(`button:has-text("Allow")`).WaitFor(); err != nil { From 1091df0cb9834c682ef97b99501cffa5cba2eb6d Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Wed, 4 Nov 2026 17:59:32 +0100 Subject: [PATCH 3/3] ci: do not cache results of tests against live staging go test cannot see changes on staging, so with the restored Go cache unchanged cloudx packages reported a cached pass without running. That kept the identifier-first login break hidden in most packages for days. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01DUBUDsgicGQri3GxrTV8VW --- .github/workflows/ci.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 67f8349c..bd8bb24c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -41,7 +41,9 @@ jobs: go run github.com/mxschmitt/playwright-go/cmd/playwright install chromium --with-deps - run: make lint - - run: go test -race ./... + # cmd/cloudx tests run against live staging, which go test cannot track, + # so a cached pass would hide a break on the staging side. + - run: go test -race -count=1 ./... env: ORY_RATE_LIMIT_HEADER: ${{ secrets.ORY_RATE_LIMIT_HEADER }} ORY_CONSOLE_URL: https://console.staging.ory.dev