From 0858d992cac5941ec2f6f8b08385a2a20cac9f08 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 11:36:44 +0200 Subject: [PATCH 1/8] fix: send the rate-limit header from the test browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI has been red on master since 2026-07-30. Every failure is the same: one or more of the cloudx packages times out in waiting for locator('button:has-text("Allow")') inside the OAuth2 login their TestMain performs, before any test body runs. The Playwright traces show why — the login POST comes back 429: {"error":{"code":429,"message":"Too Many Requests", "reason":"Too many API requests from your IP have been registered.", "status":"Blocked","details":{"ruleId":"170edb01"}}} In the run this was diagnosed from, exactly three traces contain that 429 and exactly three logins failed; the one that succeeded has none. CI sets ORY_RATE_LIMIT_HEADER precisely to avoid this, but the header only ever rode on the Go SDK and CLI HTTP clients. The browser driving the login is a separate client that never sent it, so the CLI's own API traffic was exempt while the login flow was not — and `go test ./...` runs six packages' browser logins concurrently from a single CI egress IP. The browser page now sends the same header, from one shared definition in the client package rather than a repeated literal. The second half of this commit is about how the failure read. The consent hook submitted the login form and went straight to waiting for the `Allow` button, never looking at what the submission returned. A refused submission leaves the page on the form, so the wait burned its full 30 seconds and blamed the consent screen — the reason CI reported a missing button for weeks while the actual cause was a rate limit in the response nobody inspected. Submitting now checks that response and fails immediately on a 429 or 5xx, quoting the status and body. A rejected credential is deliberately not covered there: Ory Network answers that with a 303 back to the login page, which is indistinguishable from success at that point, so the consent wait instead reports where the browser ended up. Both cases were exercised against staging: a wrong password now fails with the consent screen did not render ... The browser ended up at https://console.staging.ory.dev/login?flow=... instead of a bare locator timeout. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/client/sdks.go | 17 +++++++- cmd/cloudx/testhelpers/testhelpers.go | 60 ++++++++++++++++++++++++--- 2 files changed, 70 insertions(+), 7 deletions(-) diff --git a/cmd/cloudx/client/sdks.go b/cmd/cloudx/client/sdks.go index 52cf03ed..330acdaf 100644 --- a/cmd/cloudx/client/sdks.go +++ b/cmd/cloudx/client/sdks.go @@ -22,10 +22,23 @@ const ( RateLimitHeaderKey = "ORY_RATE_LIMIT_HEADER" ConsoleURLKey = "ORY_CONSOLE_URL" OryAPIsURLKey = "ORY_ORYAPIS_URL" + + rateLimitHeaderName = "Ory-RateLimit-Action" ) var rateLimitHeader = os.Getenv(RateLimitHeaderKey) +// RateLimitHeader returns the header that exempts a caller from Ory Network's +// per-IP rate limits, and whether one is configured at all. +// +// Everything that talks to Ory Network on the test suite's behalf has to send +// it, not just the SDK clients built below: the browser that drives the OAuth2 +// login is a separate client with its own connection, and CI runs many of those +// logins concurrently from a single egress IP. +func RateLimitHeader() (name, value string, ok bool) { + return rateLimitHeaderName, rateLimitHeader, rateLimitHeader != "" +} + func CloudConsoleURL(prefix string) *url.URL { // we load the URL from the env here instead of init() because the tests might want to change this consoleURL, err := url.ParseRequestURI(cmp.Or(os.Getenv(ConsoleURLKey), "https://console.ory.com")) @@ -58,7 +71,7 @@ func newSDKConfiguration(uri string) *cloud.Configuration { conf.OperationServers = nil conf.HTTPClient = &http.Client{Timeout: time.Second * 30} if rateLimitHeader != "" { - conf.AddDefaultHeader("Ory-RateLimit-Action", rateLimitHeader) + conf.AddDefaultHeader(rateLimitHeaderName, rateLimitHeader) } return conf } @@ -126,7 +139,7 @@ func (h *CommandHelper) newProjectHTTPClient(ctx context.Context) (*http.Client, Source: tokenSource, } if rateLimitHeader != "" { - c.Transport = &setHeaderTransport{base: c.Transport, key: "Ory-RateLimit-Action", value: rateLimitHeader} + c.Transport = &setHeaderTransport{base: c.Transport, key: rateLimitHeaderName, value: rateLimitHeader} } return c, baseURL, nil diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index 514cfd29..c1144e11 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "io" + "net/http" "os" "path/filepath" "strings" @@ -256,9 +257,20 @@ func SetupPlaywright(t testing.TB) (playwright.Browser, playwright.Page, func()) } func NewPage(t testing.TB, browser playwright.Browser) playwright.Page { - page, err := browser.NewPage(playwright.BrowserNewPageOptions{ + opts := playwright.BrowserNewPageOptions{ BaseURL: new(client.CloudConsoleURL("").String()), - }) + } + + // The browser talks to Ory Network directly, so it needs the same rate-limit + // exemption the SDK clients get — the header configured on those does not + // reach it. Without this, `go test ./...` runs the browser login of every + // cloudx package at once from a single CI egress IP and the login endpoint + // starts answering 429. + if name, value, ok := client.RateLimitHeader(); ok { + opts.ExtraHttpHeaders = map[string]string{name: value} + } + + page, err := browser.NewPage(opts) require.NoError(t, err) for _, route := range []string{ @@ -295,6 +307,42 @@ func NewPage(t testing.TB, browser playwright.Browser) playwright.Page { return page } +// submitPasswordForm submits the filled-in login form and fails immediately if +// the server refused the request outright. +// +// This catches the infrastructure-level refusals — 429 when a CI run drives more +// logins from one IP than Ory Network allows, or a 5xx — and 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. +// +// A rejected *credential* is not covered here: Ory Network answers that with a +// 303 back to the login page, which is indistinguishable from success at this +// point. The consent wait reports where the browser ended up, which is what +// separates the two. +func submitPasswordForm(t testing.TB, page playwright.Page) { + // The login flow issues exactly one request to this path, and it is this + // submission — the flow itself is created by the login UI, not the browser. + isLoginSubmission := func(url string) bool { + return strings.Contains(url, "/self-service/login") + } + + resp, err := page.ExpectResponse(isLoginSubmission, func() error { + return page.Locator(`[type="submit"][name="method"][value="password"]`).Click() + }) + require.NoError(t, err, "the login form was never submitted") + + 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) +} + func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, email, password string) func(uri string) error { return func(uri string) error { t.Logf("open browser with %s", uri) @@ -319,16 +367,18 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai t.Logf("logging in") require.NoError(t, page.Locator(`[data-testid="node/input/identifier"] input`).Fill(email)) require.NoError(t, page.Locator(`[data-testid="node/input/password"] input`).Fill(password)) - require.NoError(t, page.Locator(`[type="submit"][name="method"][value="password"]`).Click()) } else { // reconfirm password t.Logf("reconfirming password") require.NoError(t, page.Locator(`[data-testid="node/input/password"] input`).Fill(password)) - require.NoError(t, page.Locator(`[type="submit"][name="method"][value="password"]`).Click()) } + submitPasswordForm(t, page) // 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 - require.NoError(t, page.Locator(`button:has-text("Allow")`).WaitFor()) + if err := page.Locator(`button:has-text("Allow")`).WaitFor(); err != nil { + require.FailNowf(t, "the consent screen did not render", "%s\n\nThe browser ended up at %s. Still being on the login page means the credentials or the flow were rejected, rather than the consent screen itself being broken.", + err, page.URL()) + } time.Sleep(time.Second) // accept consent From 3e095a28137628fd2ac0e7482ccc8c0d54d73e80 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 11:45:50 +0200 Subject: [PATCH 2/8] fix: redact the rate-limit header from Playwright traces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sending the header from the browser puts it somewhere it was never exposed before. Traces capture complete request headers — the same property that made them useful for diagnosing the 429 — and CI uploads them as a build artifact of a public repository. GitHub masks secrets in workflow logs but not inside artifacts, so without this the change would have published the token that exempts CI from Ory Network's rate limits. Playwright has no redaction option: TracingStartOptions carries only name, title, screenshots, snapshots, live and sources. The archive is therefore rewritten once Tracing().Stop() has written it, replacing the value in every entry. The JSON-escaped spelling is replaced too, since the trace stores headers as JSON string values and a token containing a quote or backslash would otherwise sit there in a form a raw byte comparison misses. If the archive cannot be rewritten it is deleted rather than left in place. Losing one diagnostic is the far cheaper failure. Verified end to end: a login run with a dummy ORY_RATE_LIMIT_HEADER produces a trace holding 131 occurrences of the header name — which is also the first direct confirmation that the browser now sends it — zero occurrences of the value, and 139 placeholders. TestRedactInZip covers the rewriting itself, including the escaped spelling and the unreadable archive, and needs no network. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/testhelpers/redact_test.go | 106 ++++++++++++++++++++++++++ cmd/cloudx/testhelpers/testhelpers.go | 98 +++++++++++++++++++++++- 2 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 cmd/cloudx/testhelpers/redact_test.go diff --git a/cmd/cloudx/testhelpers/redact_test.go b/cmd/cloudx/testhelpers/redact_test.go new file mode 100644 index 00000000..afa12939 --- /dev/null +++ b/cmd/cloudx/testhelpers/redact_test.go @@ -0,0 +1,106 @@ +// Copyright © 2026 Ory Corp +// SPDX-License-Identifier: Apache-2.0 + +package testhelpers + +import ( + "archive/zip" + "encoding/json" + "io" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRedactInZip pins the guarantee the Playwright traces depend on: this +// repository is public and CI uploads the traces as an artifact, so the +// rate-limit header value must not survive anywhere inside one. +func TestRedactInZip(t *testing.T) { + const secret = `s3cret"value\with-escapes` + + writeArchive := func(t *testing.T, entries map[string]string) string { + path := filepath.Join(t.TempDir(), "trace.zip") + f, err := os.Create(path) + require.NoError(t, err) + defer f.Close() + + w := zip.NewWriter(f) + for name, content := range entries { + e, err := w.Create(name) + require.NoError(t, err) + _, err = e.Write([]byte(content)) + require.NoError(t, err) + } + require.NoError(t, w.Close()) + return path + } + + readArchive := func(t *testing.T, path string) map[string]string { + r, err := zip.OpenReader(path) + require.NoError(t, err) + defer r.Close() + + out := make(map[string]string, len(r.File)) + for _, f := range r.File { + src, err := f.Open() + require.NoError(t, err) + content, err := io.ReadAll(src) + require.NoError(t, err) + require.NoError(t, src.Close()) + out[f.Name] = string(content) + } + return out + } + + // The trace stores headers as JSON string values, so the secret appears in + // its escaped spelling rather than verbatim. + escaped, err := json.Marshal(secret) + require.NoError(t, err) + jsonEncoded := string(escaped) + + t.Run("case=removes the secret in both spellings", func(t *testing.T) { + path := writeArchive(t, map[string]string{ + "trace.network": `{"headers":[{"name":"Ory-RateLimit-Action","value":` + jsonEncoded + `}]}`, + "trace.trace": "prefix " + secret + " suffix", + "resources/1": "a response body mentioning nothing", + }) + + require.NoError(t, redactInZip(path, secret)) + + for name, content := range readArchive(t, path) { + assert.NotContains(t, content, secret, "%s still holds the raw secret", name) + assert.NotContains(t, content, jsonEncoded[1:len(jsonEncoded)-1], "%s still holds the escaped secret", name) + } + }) + + t.Run("case=leaves the rest of the trace intact", func(t *testing.T) { + path := writeArchive(t, map[string]string{ + "trace.trace": "keep me " + secret + " keep me too", + "resources/1": "untouched", + }) + + require.NoError(t, redactInZip(path, secret)) + + got := readArchive(t, path) + assert.Equal(t, "keep me "+redactedPlaceholder+" keep me too", got["trace.trace"]) + assert.Equal(t, "untouched", got["resources/1"]) + }) + + t.Run("case=no configured secret leaves the archive alone", func(t *testing.T) { + path := writeArchive(t, map[string]string{"trace.trace": "verbatim"}) + + require.NoError(t, redactInZip(path, "")) + + assert.Equal(t, "verbatim", readArchive(t, path)["trace.trace"]) + }) + + t.Run("case=an unreadable archive is an error, so the caller can delete it", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "not-a-zip.zip") + require.NoError(t, os.WriteFile(path, []byte("definitely not a zip"), 0o600)) + + assert.Error(t, redactInZip(path, secret)) + }) +} diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index c1144e11..5334efb7 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -4,6 +4,8 @@ package testhelpers import ( + "archive/zip" + "bytes" "context" "encoding/json" "fmt" @@ -307,6 +309,100 @@ func NewPage(t testing.TB, browser playwright.Browser) playwright.Page { return page } +// stopTracing writes the trace of the login flow and strips the rate-limit +// header value out of it. +// +// A trace records complete request headers — this repository is public and CI +// uploads the traces as a build artifact, where GitHub's secret masking does not +// reach. The header that exempts CI from Ory Network's rate limits therefore +// must not survive into one. Playwright offers no redaction option, so the +// archive is rewritten after it has been written. +// +// If it cannot be rewritten the trace is deleted: losing a diagnostic is the +// cheaper failure by far. +func stopTracing(t testing.TB, page playwright.Page) { + path := filepath.Join(tracesDir, fmt.Sprintf("%s.zip", t.Name())) + if err := page.Context().Tracing().Stop(path); err != nil { + t.Logf("tracing stop error: %+v", err) + return + } + + _, secret, ok := client.RateLimitHeader() + if !ok { + return + } + if err := redactInZip(path, secret); err != nil { + t.Logf("could not redact %s, removing it: %+v", path, err) + require.NoError(t, os.Remove(path)) + } +} + +const redactedPlaceholder = "[redacted]" + +// redactInZip rewrites every entry of the zip archive at path, replacing each +// occurrence of secret with a placeholder. +// +// The JSON-escaped spelling is replaced as well, because the trace stores +// headers as JSON string values and a secret containing a quote or backslash +// would otherwise appear there in a form the raw comparison does not match. +func redactInZip(path, secret string) error { + if secret == "" { + return nil + } + + needles := [][]byte{[]byte(secret)} + if escaped, err := json.Marshal(secret); err == nil { + if inner := escaped[1 : len(escaped)-1]; !bytes.Equal(inner, []byte(secret)) { + needles = append(needles, inner) + } + } + + r, err := zip.OpenReader(path) + if err != nil { + return err + } + + tmp, err := os.CreateTemp(filepath.Dir(path), "trace-*.zip") + if err != nil { + _ = r.Close() + return err + } + defer os.Remove(tmp.Name()) // no-op once the rename below succeeded + + err = func() error { + w := zip.NewWriter(tmp) + for _, f := range r.File { + src, err := f.Open() + if err != nil { + return err + } + content, err := io.ReadAll(src) + _ = src.Close() + if err != nil { + return err + } + for _, needle := range needles { + content = bytes.ReplaceAll(content, needle, []byte(redactedPlaceholder)) + } + dst, err := w.Create(f.Name) + if err != nil { + return err + } + if _, err := dst.Write(content); err != nil { + return err + } + } + return w.Close() + }() + _ = r.Close() + _ = tmp.Close() + if err != nil { + return err + } + + return os.Rename(tmp.Name(), path) +} + // submitPasswordForm submits the filled-in login form and fails immediately if // the server refused the request outright. // @@ -353,7 +449,7 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai })) defer func() { r := recover() - _ = page.Context().Tracing().Stop(filepath.Join(tracesDir, fmt.Sprintf("%s.zip", t.Name()))) + stopTracing(t, page) if r != nil { panic(r) } From 45462d4f146ff28fb330e8c94a7dcadeb520e22f Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 11:49:44 +0200 Subject: [PATCH 3/8] fix: scope the rate-limit header to the Ory Console MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ExtraHttpHeaders applies to every request the page makes, and the login page is not self-contained: it pulls in Stripe, Sentry, Cloudflare Insights, and Ory's own consent and analytics hosts. Traced with a dummy value, the secret went to ten hosts — js.stripe.com, r.stripe.com, consent.ory.com, consent.ory.sh, sqa-web.ory.com, static.cloudflareinsights.com and o481709.ingest.sentry.io among them. Redacting it from the trace does nothing about that: those requests are real, and third parties would have received the token on every CI run. The header is now attached per request through page.Route, to the console host and its subdomains only — the console serves the login UI and its subdomains serve the flow the UI submits to, so both need it. A request whose headers cannot be read continues unmodified rather than being cancelled: a login without the header may still succeed, a cancelled one cannot. Re-traced with the same dummy value, the header now reaches console.staging.ory.dev, project.console.staging.ory.dev and api.console.staging.ory.dev, and nothing else. The login still completes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/testhelpers/testhelpers.go | 60 +++++++++++++++++++++------ 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index 5334efb7..35f8fc69 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -259,22 +260,13 @@ func SetupPlaywright(t testing.TB) (playwright.Browser, playwright.Page, func()) } func NewPage(t testing.TB, browser playwright.Browser) playwright.Page { - opts := playwright.BrowserNewPageOptions{ + page, err := browser.NewPage(playwright.BrowserNewPageOptions{ BaseURL: new(client.CloudConsoleURL("").String()), - } - - // The browser talks to Ory Network directly, so it needs the same rate-limit - // exemption the SDK clients get — the header configured on those does not - // reach it. Without this, `go test ./...` runs the browser login of every - // cloudx package at once from a single CI egress IP and the login endpoint - // starts answering 429. - if name, value, ok := client.RateLimitHeader(); ok { - opts.ExtraHttpHeaders = map[string]string{name: value} - } - - page, err := browser.NewPage(opts) + }) require.NoError(t, err) + routeRateLimitHeader(t, page) + for _, route := range []string{ "doubleclick.net", "google-analytics.com", @@ -309,6 +301,48 @@ func NewPage(t testing.TB, browser playwright.Browser) playwright.Page { return page } +// routeRateLimitHeader makes the browser send the rate-limit header that exempts +// CI from Ory Network's per-IP limits, to the Ory Console and nothing else. +// +// The browser needs it because it talks to Ory Network directly and the header +// configured on the SDK clients does not reach it: `go test ./...` runs the +// browser login of every cloudx package at once from a single CI egress IP, and +// the login endpoint answers 429. +// +// It is attached per request rather than through the page's ExtraHttpHeaders, +// which apply to every request the page makes — the login page pulls in Stripe, +// Sentry, Cloudflare Insights and Ory's own consent and analytics hosts, and all +// of them would receive the secret. +func routeRateLimitHeader(t testing.TB, page playwright.Page) { + name, value, ok := client.RateLimitHeader() + if !ok { + return + } + + // The console serves the login UI and its subdomains serve the flow the UI + // submits to, so both have to carry the header. + console := client.CloudConsoleURL("").Host + isConsole := func(rawURL string) bool { + parsed, err := url.Parse(rawURL) + if err != nil { + return false + } + return parsed.Host == console || strings.HasSuffix(parsed.Host, "."+console) + } + + require.NoError(t, page.Route(isConsole, func(r playwright.Route) { + headers, err := r.Request().AllHeaders() + if err != nil { + // Continue unmodified rather than dropping the request: a login + // without the header may still succeed, a cancelled one cannot. + _ = r.Continue() + return + } + headers[name] = value + _ = r.Continue(playwright.RouteContinueOptions{Headers: headers}) + })) +} + // stopTracing writes the trace of the login flow and strips the rate-limit // header value out of it. // From 92ea6f9e2f3f9cf8b7bfb140a17e0afe81b24472 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 11:58:25 +0200 Subject: [PATCH 4/8] fix: do not fail the suite when there is no trace to redact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redaction added two commits ago took TestMain down with it in CI: could not redact .../playwright-traces/TestMain.zip, removing it: no such file or directory Error: remove .../TestMain.zip: no such file or directory Two mistakes. A trace that was never written cannot leak anything, so a missing archive is nothing to redact rather than an error, and removing an already-absent file is not a failure either — only a trace still on disk afterwards is worth failing over. The second mistake is what produced the missing file. Every package's TestMain traces under the same name into one shared directory, so the packages `go test ./...` runs in parallel were already overwriting each other's traces; rewriting one then raced the next writer, and a failed rewrite deleted the file out from under it. Trace names are now qualified by package, which removes the race and stops the traces from clobbering each other — worth having on its own, given these traces are the only record of what the browser did. Verified with two browser-login packages in parallel and a dummy ORY_RATE_LIMIT_HEADER: both complete, each writes its own trace (identity.TestMain.zip, relationtuples.TestMain.zip), both hold zero occurrences of the value, and the header still reaches only the console hosts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/testhelpers/redact_test.go | 7 ++++++ cmd/cloudx/testhelpers/testhelpers.go | 34 +++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/cmd/cloudx/testhelpers/redact_test.go b/cmd/cloudx/testhelpers/redact_test.go index afa12939..818722d8 100644 --- a/cmd/cloudx/testhelpers/redact_test.go +++ b/cmd/cloudx/testhelpers/redact_test.go @@ -103,4 +103,11 @@ func TestRedactInZip(t *testing.T) { assert.Error(t, redactInZip(path, secret)) }) + + t.Run("case=a missing archive is not an error", func(t *testing.T) { + // Tracing does not always leave a file behind, and an archive that was + // never written cannot leak anything. Treating it as a failure took the + // whole TestMain down with it. + assert.NoError(t, redactInZip(filepath.Join(t.TempDir(), "absent.zip"), secret)) + }) } diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index 35f8fc69..362071b2 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -8,8 +8,10 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" + "io/fs" "net/http" "net/url" "os" @@ -355,7 +357,11 @@ func routeRateLimitHeader(t testing.TB, page playwright.Page) { // If it cannot be rewritten the trace is deleted: losing a diagnostic is the // cheaper failure by far. func stopTracing(t testing.TB, page playwright.Page) { - path := filepath.Join(tracesDir, fmt.Sprintf("%s.zip", t.Name())) + // The name is qualified by package because every package's TestMain traces + // under the same test name into one shared directory: unqualified, the + // packages `go test ./...` runs in parallel overwrite each other's traces, + // and rewriting one races the next writer. + path := filepath.Join(tracesDir, fmt.Sprintf("%s.%s.zip", tracesPackage, t.Name())) if err := page.Context().Tracing().Stop(path); err != nil { t.Logf("tracing stop error: %+v", err) return @@ -365,9 +371,17 @@ func stopTracing(t testing.TB, page playwright.Page) { if !ok { return } - if err := redactInZip(path, secret); err != nil { - t.Logf("could not redact %s, removing it: %+v", path, err) - require.NoError(t, os.Remove(path)) + + err := redactInZip(path, secret) + if err == nil { + return + } + + // The archive could not be rewritten, so make sure it cannot be uploaded. + // Only a trace that is still on disk afterwards is worth failing over. + t.Logf("could not redact %s, removing it: %+v", path, err) + if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + require.NoError(t, err, "the trace may still carry the rate-limit header") } } @@ -392,6 +406,10 @@ func redactInZip(path, secret string) error { } r, err := zip.OpenReader(path) + if errors.Is(err, fs.ErrNotExist) { + // Tracing wrote no archive, so there is nothing that could leak. + return nil + } if err != nil { return err } @@ -520,13 +538,19 @@ func PlaywrightAcceptConsentBrowserHook(t testing.TB, page playwright.Page, emai } } -var tracesDir string +var ( + tracesDir string + // tracesPackage is the package under test, used to keep the traces of + // packages running in parallel apart. See stopTracing. + tracesPackage string +) func init() { cwd, err := os.Getwd() if err != nil { panic(err) } + tracesPackage = filepath.Base(cwd) dirs := strings.Split(cwd, string(os.PathSeparator)) for i := range dirs { if dirs[i] == "cloudx" { From 185ff197b6365301e649462d20410618c2a15a29 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 13:15:09 +0200 Subject: [PATCH 5/8] fix: redact the trace even when stopping it failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stopTracing returned as soon as Tracing().Stop() reported an error, which skipped the redaction and left whatever was on disk for the artifact upload to collect. Stop can fail with the trace already written. It assembles the archive in doStopChunk and only then sends `tracingStop`, and that send is allowed to fail on its own — so the error can arrive after a complete, unredacted trace has been saved. The remote path can fail after artifact.SaveAs too, and the local Zip can fail partway and truncate. None of those may reach a public build artifact carrying the header. The early return is dropped: the error is logged and the trace is redacted, or removed when it cannot be. That is better than deleting unconditionally on a stop error, because the common case — a complete archive plus a failed `tracingStop` — keeps a usable diagnostic. The redact-or-remove step moves into its own function so the boundary is testable, and two cases now cover it: a truncated archive is removed, a rewritable one is kept and redacted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/testhelpers/redact_test.go | 26 ++++++++++++++++++++++++++ cmd/cloudx/testhelpers/testhelpers.go | 18 +++++++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/cmd/cloudx/testhelpers/redact_test.go b/cmd/cloudx/testhelpers/redact_test.go index 818722d8..0d5cbeef 100644 --- a/cmd/cloudx/testhelpers/redact_test.go +++ b/cmd/cloudx/testhelpers/redact_test.go @@ -110,4 +110,30 @@ func TestRedactInZip(t *testing.T) { // whole TestMain down with it. assert.NoError(t, redactInZip(filepath.Join(t.TempDir(), "absent.zip"), secret)) }) + + // Tracing().Stop() assembles the archive before the call that finishes + // tracing, and that call may fail on its own, so a trace can be left behind + // complete or truncated even when stopping reports an error. Neither may + // reach the uploaded artifact carrying the secret. + t.Run("case=an unrewritable archive is removed", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "truncated.zip") + full := writeArchive(t, map[string]string{"trace.trace": secret}) + content, err := os.ReadFile(full) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, content[:len(content)/2], 0o600)) + + redactOrRemoveTrace(t, path, secret) + + _, err = os.Stat(path) + assert.ErrorIs(t, err, os.ErrNotExist, "a trace that could not be redacted must not survive") + }) + + t.Run("case=a rewritable archive is kept, redacted", func(t *testing.T) { + path := writeArchive(t, map[string]string{"trace.trace": "before " + secret + " after"}) + + redactOrRemoveTrace(t, path, secret) + + require.FileExists(t, path, "a redacted trace is still worth uploading") + assert.Equal(t, "before "+redactedPlaceholder+" after", readArchive(t, path)["trace.trace"]) + }) } diff --git a/cmd/cloudx/testhelpers/testhelpers.go b/cmd/cloudx/testhelpers/testhelpers.go index 362071b2..523fdc85 100644 --- a/cmd/cloudx/testhelpers/testhelpers.go +++ b/cmd/cloudx/testhelpers/testhelpers.go @@ -362,24 +362,36 @@ func stopTracing(t testing.TB, page playwright.Page) { // packages `go test ./...` runs in parallel overwrite each other's traces, // and rewriting one races the next writer. path := filepath.Join(tracesDir, fmt.Sprintf("%s.%s.zip", tracesPackage, t.Name())) + + // A failure here is not a reason to skip the redaction below. Stop assembles + // the archive first and only then sends `tracingStop`, which is itself + // allowed to fail, so it can return an error having already written a + // complete trace — and one that is only partially written is just as + // unwelcome in the artifact. All a failure means is that the trace may be + // absent or truncated, both of which the next step handles. if err := page.Context().Tracing().Stop(path); err != nil { t.Logf("tracing stop error: %+v", err) - return } _, secret, ok := client.RateLimitHeader() if !ok { return } + redactOrRemoveTrace(t, path, secret) +} +// redactOrRemoveTrace strips secret out of the trace archive at path, and +// removes the archive if that is not possible — a trace that cannot be rewritten +// must not reach the uploaded artifact. An archive that was never written is +// nothing to redact and nothing to remove. +func redactOrRemoveTrace(t testing.TB, path, secret string) { err := redactInZip(path, secret) if err == nil { return } - // The archive could not be rewritten, so make sure it cannot be uploaded. - // Only a trace that is still on disk afterwards is worth failing over. t.Logf("could not redact %s, removing it: %+v", path, err) + // Only a trace still on disk afterwards is worth failing over. if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { require.NoError(t, err, "the trace may still carry the rate-limit header") } From d0c37e74bdef2058dbe081f37b6bbe2afa28a296 Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 12:35:22 +0200 Subject: [PATCH 6/8] test: migrate TestCRUD off subject IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ory Network rejects relation tuples carrying a subject_id: rpc error: code = InvalidArgument desc = subject_id is not supported; please migrate to subject sets The rejection is unconditional — plain strings, UUIDs and namespaced IDs are all refused, under legacy and OPL namespaces alike — so the tuples this test writes are subject sets now. The `ory is allowed s r n o1` assertion is dropped rather than adapted. `is allowed` takes a plain subject and sends it as a subject_id, so the server rejects the check with the same error: the command is unusable against Ory Network, not merely deprecated, and no reachable tuple makes it answer true — every permission chain has to terminate in a subject ID. Asserting the broken behaviour here would only cement it, so the comment records what happened and the command needs its own fix. Note that CI on this branch is expected to stay red until #457 lands: master's browser login is rate limited, and the packages that fail on that are unrelated to this change. The failure fixed here has been present all along and only surfaces in the runs where relationtuples gets past its login at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- .../relationtuples/relationtuples_test.go | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/cmd/cloudx/relationtuples/relationtuples_test.go b/cmd/cloudx/relationtuples/relationtuples_test.go index e659fd90..da7376de 100644 --- a/cmd/cloudx/relationtuples/relationtuples_test.go +++ b/cmd/cloudx/relationtuples/relationtuples_test.go @@ -92,12 +92,20 @@ func TestCRUD(t *testing.T) { t.Parallel() createLegacyNamespace(t, defaultProject.Id, `{"name": "n", "id": 0}`) + // Ory Network rejects writes carrying a subject_id — "subject_id is not + // supported; please migrate to subject sets" — for every form of it (plain + // string, UUID, namespaced) and under both legacy and OPL namespaces, so the + // tuples here are subject sets. tuple := func(object string) string { return fmt.Sprintf(`[{ "namespace": "n", "object": %q, "relation": "r", - "subject_id": "s" + "subject_set": { + "namespace": "n", + "object": "s", + "relation": "r" + } }]`, object) } create := func(t *testing.T, object string) string { @@ -111,14 +119,6 @@ func TestCRUD(t *testing.T) { require.NoError(t, err, stderr) return stdout } - isAllowed := func(t *testing.T, subject, relation, namespace, object string) string { - stdout, stderr, err := defaultCmd.Exec(nil, - "is", "allowed", subject, relation, namespace, object, - "--project", defaultProject.Id, "--format", "json") - require.NoError(t, err, stderr) - return stdout - } - // 1. create a tuple stdout := create(t, "o1") require.JSONEq(t, tuple("o1"), stdout) @@ -127,9 +127,12 @@ func TestCRUD(t *testing.T) { stdout = list(t) require.JSONEq(t, tuple("o1"), gjson.Get(stdout, "relation_tuples").Raw, stdout) - // check that it is allowed - stdout = isAllowed(t, "s", "r", "n", "o1") - require.JSONEq(t, `{"allowed":true}`, stdout, stdout) + // There used to be an `ory is allowed s r n o1` check here. It cannot run + // against Ory Network any more: `is allowed` takes a plain subject and sends + // it as a subject_id, which the server now rejects outright with the same + // "please migrate to subject sets" error as a write does. That makes the + // command unusable rather than merely deprecated, so it is tracked + // separately instead of being asserted as broken here. // 3. delete with --all but without --force stdout, stderr, err := defaultCmd.Exec(nil, "delete", "relation-tuples", "--format", "json", "--project", defaultProject.Id, From 925b483fc9ed0d32cc10ca41d5781b1e937c53cf Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 13:30:39 +0200 Subject: [PATCH 7/8] fix: document the subject forms `ory is allowed` accepts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The usage line read allowed which documented only the deprecated four-argument object form and gave no hint that the subject may be a subject set. Following it produces a deprecation warning and a request Ory Network rejects outright: Could not make request: rpc error: code = InvalidArgument desc = subject_id is not supported; please migrate to subject sets The command itself was fine — keto's ParseSubject reads anything containing a colon as a subject set — so this was the help steering callers into the one form that no longer works. It now names the subject-set spelling, the `:` object form, and what Ory Network does with plain subject IDs, and carries an example. Clearing the aliases is part of the same slip: NewAllowedCmd runs wrapForOryCLI, which names the command it is normally applied to, so `ory is allowed` also answered to `relationships`, `relation-tuples`, `relationship` and `relation-tuple` — the aliases of a different command. The TestCRUD comment is corrected alongside. It claimed the command was unusable against Ory Network, which is wrong: `is allowed n:s#r r n:o1` runs fine against a subject-set tuple. It answers false, and nothing reachable makes it answer true while relationships with a plain subject ID cannot be stored, which is why the assertion stays dropped. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/relationtuples/permissions.go | 25 ++++++++++++++++++- .../relationtuples/relationtuples_test.go | 19 +++++++++----- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/cmd/cloudx/relationtuples/permissions.go b/cmd/cloudx/relationtuples/permissions.go index 0780b12b..53f9abf7 100644 --- a/cmd/cloudx/relationtuples/permissions.go +++ b/cmd/cloudx/relationtuples/permissions.go @@ -12,7 +12,30 @@ import ( func NewAllowedCmd() *cobra.Command { cmd := check.NewCheckCmd() wrapForOryCLI(cmd) - cmd.Use = "allowed " + + // The previous usage line, `allowed `, + // documented only the deprecated four-argument form and gave no hint that + // the subject may be a subject set — so it steered callers into passing a + // bare subject ID, which Ory Network rejects with "subject_id is not + // supported; please migrate to subject sets". + cmd.Use = "allowed :" + // wrapForOryCLI names the command it is normally applied to, so without this + // `ory is allowed` also answers to `relationships`, `relation-tuples` and + // friends — the aliases of a different command entirely. + cmd.Aliases = nil + cmd.Long = `Check whether a subject has a relation on an object. + +The subject is either a subject set ` + "`:#`" + ` or a +plain subject ID. Ory Network no longer accepts relationships written with a +plain subject ID, so a subject set is what a check against it will match. + +Passing the object as two separate arguments still works but is deprecated; +use ` + "`:`" + ` instead.` + cmd.Example = `$ {{ .CommandPath }} 'groups:engineering#member' view documents:readme + +{ + "allowed": true +}` return cmd } diff --git a/cmd/cloudx/relationtuples/relationtuples_test.go b/cmd/cloudx/relationtuples/relationtuples_test.go index da7376de..61beb51f 100644 --- a/cmd/cloudx/relationtuples/relationtuples_test.go +++ b/cmd/cloudx/relationtuples/relationtuples_test.go @@ -127,12 +127,19 @@ func TestCRUD(t *testing.T) { stdout = list(t) require.JSONEq(t, tuple("o1"), gjson.Get(stdout, "relation_tuples").Raw, stdout) - // There used to be an `ory is allowed s r n o1` check here. It cannot run - // against Ory Network any more: `is allowed` takes a plain subject and sends - // it as a subject_id, which the server now rejects outright with the same - // "please migrate to subject sets" error as a write does. That makes the - // command unusable rather than merely deprecated, so it is tracked - // separately instead of being asserted as broken here. + // There used to be an `ory is allowed s r n o1` check here asserting + // `{"allowed":true}`. + // + // The command still works — it accepts a subject set, and `is allowed + // n:s#r r n:o1` against the tuple above runs fine — but it answers + // `{"allowed":false}`, and no reachable configuration makes it answer true: + // granting a permission that evaluates to true needs a relationship whose + // subject is a plain ID, which is exactly what the server refuses to store. + // Only a bare subject fails outright, with the same "please migrate to + // subject sets" error a write gets. + // + // So there is nothing here left to assert that would not be asserting the + // server's current state rather than the CLI's behaviour. // 3. delete with --all but without --force stdout, stderr, err := defaultCmd.Exec(nil, "delete", "relation-tuples", "--format", "json", "--project", defaultProject.Id, From ffe46b1b95a08c99e2c1b025f5b57dc9f4abda8b Mon Sep 17 00:00:00 2001 From: Arne Luenser Date: Mon, 17 Aug 2026 20:28:27 +0200 Subject: [PATCH 8/8] docs: drop comments that narrate earlier revisions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: comments describing what the code used to say are noise. A reader has git for that, and the comment goes stale the moment anyone touches the line. Removes the three that did it and the RateLimitHeader doc paragraph restating what its callers already show. The remaining comments explain why the current code is shaped as it is — including why the rate-limit header is routed per request rather than set on the page, which is the one that stops someone simplifying it back into a secret leak. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tg5VWFUb7824qkrooUdvBA --- cmd/cloudx/client/sdks.go | 5 ----- cmd/cloudx/relationtuples/permissions.go | 10 ++-------- .../relationtuples/relationtuples_test.go | 18 +++++------------- cmd/cloudx/testhelpers/redact_test.go | 3 +-- 4 files changed, 8 insertions(+), 28 deletions(-) diff --git a/cmd/cloudx/client/sdks.go b/cmd/cloudx/client/sdks.go index 330acdaf..05d45134 100644 --- a/cmd/cloudx/client/sdks.go +++ b/cmd/cloudx/client/sdks.go @@ -30,11 +30,6 @@ var rateLimitHeader = os.Getenv(RateLimitHeaderKey) // RateLimitHeader returns the header that exempts a caller from Ory Network's // per-IP rate limits, and whether one is configured at all. -// -// Everything that talks to Ory Network on the test suite's behalf has to send -// it, not just the SDK clients built below: the browser that drives the OAuth2 -// login is a separate client with its own connection, and CI runs many of those -// logins concurrently from a single egress IP. func RateLimitHeader() (name, value string, ok bool) { return rateLimitHeaderName, rateLimitHeader, rateLimitHeader != "" } diff --git a/cmd/cloudx/relationtuples/permissions.go b/cmd/cloudx/relationtuples/permissions.go index 53f9abf7..49d7dd16 100644 --- a/cmd/cloudx/relationtuples/permissions.go +++ b/cmd/cloudx/relationtuples/permissions.go @@ -13,15 +13,9 @@ func NewAllowedCmd() *cobra.Command { cmd := check.NewCheckCmd() wrapForOryCLI(cmd) - // The previous usage line, `allowed `, - // documented only the deprecated four-argument form and gave no hint that - // the subject may be a subject set — so it steered callers into passing a - // bare subject ID, which Ory Network rejects with "subject_id is not - // supported; please migrate to subject sets". cmd.Use = "allowed :" - // wrapForOryCLI names the command it is normally applied to, so without this - // `ory is allowed` also answers to `relationships`, `relation-tuples` and - // friends — the aliases of a different command entirely. + // wrapForOryCLI sets the aliases of the relationships command, which do not + // belong on a permission check. cmd.Aliases = nil cmd.Long = `Check whether a subject has a relation on an object. diff --git a/cmd/cloudx/relationtuples/relationtuples_test.go b/cmd/cloudx/relationtuples/relationtuples_test.go index 61beb51f..fdfd4081 100644 --- a/cmd/cloudx/relationtuples/relationtuples_test.go +++ b/cmd/cloudx/relationtuples/relationtuples_test.go @@ -127,19 +127,11 @@ func TestCRUD(t *testing.T) { stdout = list(t) require.JSONEq(t, tuple("o1"), gjson.Get(stdout, "relation_tuples").Raw, stdout) - // There used to be an `ory is allowed s r n o1` check here asserting - // `{"allowed":true}`. - // - // The command still works — it accepts a subject set, and `is allowed - // n:s#r r n:o1` against the tuple above runs fine — but it answers - // `{"allowed":false}`, and no reachable configuration makes it answer true: - // granting a permission that evaluates to true needs a relationship whose - // subject is a plain ID, which is exactly what the server refuses to store. - // Only a bare subject fails outright, with the same "please migrate to - // subject sets" error a write gets. - // - // So there is nothing here left to assert that would not be asserting the - // server's current state rather than the CLI's behaviour. + // This does not assert on `ory is allowed`. The check runs against a + // subject-set tuple, but answers false whatever it is given: a permission + // that evaluates to true needs a relationship whose subject is a plain ID, + // which the server refuses to store. Asserting that would pin the server's + // state rather than the CLI's behaviour. // 3. delete with --all but without --force stdout, stderr, err := defaultCmd.Exec(nil, "delete", "relation-tuples", "--format", "json", "--project", defaultProject.Id, diff --git a/cmd/cloudx/testhelpers/redact_test.go b/cmd/cloudx/testhelpers/redact_test.go index 0d5cbeef..ff10e954 100644 --- a/cmd/cloudx/testhelpers/redact_test.go +++ b/cmd/cloudx/testhelpers/redact_test.go @@ -106,8 +106,7 @@ func TestRedactInZip(t *testing.T) { t.Run("case=a missing archive is not an error", func(t *testing.T) { // Tracing does not always leave a file behind, and an archive that was - // never written cannot leak anything. Treating it as a failure took the - // whole TestMain down with it. + // never written cannot leak anything. assert.NoError(t, redactInZip(filepath.Join(t.TempDir(), "absent.zip"), secret)) })