From 06f2e003ea9648b73af3eaae830a6b612dea69db Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 08:58:29 +0100 Subject: [PATCH 01/12] refactor(auth): give the missing discovery document a sentinel The 404 from /.well-known/oauth-authorization-server carried its own recovery guidance ("is this a Flagsmith API URL?"). Hints don't live in message strings, and this one is about to become conditional: an old Flagsmith needs a different answer from a URL that isn't Flagsmith at all. Wrap ErrNoDiscovery instead, and map it to hintAPIURL alongside the existing hintSDKAPIURL. beep boop --- internal/auth/oauth.go | 6 +++++- internal/auth/oauth_test.go | 4 ++-- internal/cmd/errors.go | 3 +++ internal/cmd/errors_test.go | 2 ++ internal/cmd/sentinel_test.go | 1 + 5 files changed, 13 insertions(+), 3 deletions(-) diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index e6ff2b7..6c9cbf2 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -127,6 +127,10 @@ func secureScheme(u *url.URL) bool { return ip != nil && ip.IsLoopback() } +// ErrNoDiscovery reports that an instance serves no authorization server +// metadata: a Flagsmith predating the OAuth login, or not a Flagsmith API. +var ErrNoDiscovery = errors.New("no authorization server metadata") + func Discover(ctx context.Context, httpClient *http.Client, apiURL string) (*Metadata, error) { u := strings.TrimRight(apiURL, "/") + "/.well-known/oauth-authorization-server" req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil) @@ -139,7 +143,7 @@ func Discover(ctx context.Context, httpClient *http.Client, apiURL string) (*Met } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("%s returned %s — is this a Flagsmith API URL?", u, resp.Status) + return nil, fmt.Errorf("%s returned %s: %w", u, resp.Status, ErrNoDiscovery) } var md Metadata if err := json.NewDecoder(resp.Body).Decode(&md); err != nil { diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 659c53b..8728ce8 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -416,8 +416,8 @@ func TestDiscover(t *testing.T) { _, err := Discover(context.Background(), http.DefaultClient, srv.URL) // Then - if err == nil || !strings.Contains(err.Error(), "is this a Flagsmith API URL?") { - t.Errorf("err = %v, want a helpful not-Flagsmith hint", err) + if !errors.Is(err, ErrNoDiscovery) { + t.Errorf("err = %v, want ErrNoDiscovery", err) } }) diff --git a/internal/cmd/errors.go b/internal/cmd/errors.go index 78fdb3d..330a27c 100644 --- a/internal/cmd/errors.go +++ b/internal/cmd/errors.go @@ -29,6 +29,7 @@ const ( hintEnvironmentKey = "Check FLAGSMITH_ENVIRONMENT_KEY, or the environment name/key passed with -e." hintSDKAPIURL = "Check --sdk-api-url (or `sdkApiUrl`) — it must point at a Flagsmith SDK API." + hintAPIURL = "Check --api-url (or `apiUrl`) — it must point at a Flagsmith API." hintEnvironmentList = "Run `flagsmith environment list` to see the environments in this project." hintProjectList = "Run `flagsmith project list` to see the projects you can access." @@ -81,6 +82,8 @@ func hintFor(err error) string { switch { case errors.Is(err, auth.ErrNotLoggedIn): return hintLogin + case errors.Is(err, auth.ErrNoDiscovery): + return hintAPIURL case errors.Is(err, auth.ErrRefreshFailed): return hintRelogin case errors.Is(err, auth.ErrKeychainUnavailable): diff --git a/internal/cmd/errors_test.go b/internal/cmd/errors_test.go index 20cfbf6..b7c3587 100644 --- a/internal/cmd/errors_test.go +++ b/internal/cmd/errors_test.go @@ -22,6 +22,8 @@ func TestHintFor(t *testing.T) { want string }{ {"not logged in", auth.ErrNotLoggedIn, hintLogin}, + {"no discovery document", auth.ErrNoDiscovery, hintAPIURL}, + {"no discovery document wrapped", fmt.Errorf("%s returned 404: %w", "https://x/.well-known", auth.ErrNoDiscovery), hintAPIURL}, {"plan gated", api.ErrPlanGated, hintPricing}, {"plan gated wrapped", fmt.Errorf("create project: %w", api.ErrPlanGated), hintPricing}, {"quota exceeded", api.ErrQuotaExceeded, hintQuota}, diff --git a/internal/cmd/sentinel_test.go b/internal/cmd/sentinel_test.go index 2a5db67..375bcb4 100644 --- a/internal/cmd/sentinel_test.go +++ b/internal/cmd/sentinel_test.go @@ -26,6 +26,7 @@ func TestEverySentinelHasAHintDecision(t *testing.T) { "api.ErrQuotaExceeded": api.ErrQuotaExceeded, "api.ErrWorkflowGated": api.ErrWorkflowGated, "auth.ErrNotLoggedIn": auth.ErrNotLoggedIn, + "auth.ErrNoDiscovery": auth.ErrNoDiscovery, "auth.ErrKeychainUnavailable": auth.ErrKeychainUnavailable, "auth.ErrRefreshFailed": auth.ErrRefreshFailed, "auth.ErrServerSideKey": auth.ErrServerSideKey, From e6f0a28fb829d68f6929917dd0869d1db99e0fe4 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 08:59:29 +0100 Subject: [PATCH 02/12] feat(api): read an instance's version from /version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answers what a Flagsmith supports before there is a credential to ask with, which is exactly when the CLI needs to know: the login flow has to tell an instance too old for OAuth apart from a URL that is not Flagsmith. Root-scoped, not under /api/v1, and unauthenticated — so a package function rather than a Client method, whose newRequest would need auth that does not exist yet at this point in the flow. beep boop --- internal/api/version.go | 42 +++++++++++++++++++ internal/api/version_test.go | 79 ++++++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 internal/api/version.go create mode 100644 internal/api/version_test.go diff --git a/internal/api/version.go b/internal/api/version.go new file mode 100644 index 0000000..bb96870 --- /dev/null +++ b/internal/api/version.go @@ -0,0 +1,42 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "strings" +) + +// ServerVersion reports the version of the Flagsmith serving apiURL, from the +// unauthenticated /version endpoint — it answers what an instance supports +// before there is any credential to ask with. The value is the deployed image +// tag, which self-hosted instances often set to something that is not a version +// at all ("latest", a commit sha), so callers must read an unparseable result as +// unknown rather than old. +func ServerVersion(ctx context.Context, httpClient *http.Client, apiURL string) (string, error) { + u := strings.TrimRight(apiURL, "/") + "/version/" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil) + if err != nil { + return "", err + } + resp, err := httpClient.Do(req) + if err != nil { + return "", fmt.Errorf("reaching %s: %w", u, err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("%s returned %s", u, resp.Status) + } + var doc struct { + ImageTag string `json:"image_tag"` + } + if err := json.NewDecoder(resp.Body).Decode(&doc); err != nil { + return "", fmt.Errorf("decoding %s: %w", u, err) + } + if doc.ImageTag == "" { + return "", errors.New(u + " carries no image tag") + } + return doc.ImageTag, nil +} diff --git a/internal/api/version_test.go b/internal/api/version_test.go new file mode 100644 index 0000000..fda678b --- /dev/null +++ b/internal/api/version_test.go @@ -0,0 +1,79 @@ +package api + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +func TestServerVersion(t *testing.T) { + t.Run("reports the image tag", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/version/" { + t.Errorf("path = %q", r.URL.Path) + } + fmt.Fprint(w, `{"ci_commit_sha":"67853f3","image_tag":"2.262.0","is_saas":true,"package_versions":{".":"2.262.0"}}`) + })) + defer srv.Close() + + // When + got, err := ServerVersion(context.Background(), srv.Client(), srv.URL+"/") + + // Then + if err != nil { + t.Fatal(err) + } + if got != "2.262.0" { + t.Errorf("ServerVersion = %q, want 2.262.0", got) + } + }) + + t.Run("errors when the endpoint is absent", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.NotFoundHandler()) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) + + t.Run("errors when the body is not the version document", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, "nope") + })) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) + + t.Run("errors when the tag is missing", func(t *testing.T) { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, `{"ci_commit_sha":"67853f3"}`) + })) + defer srv.Close() + + // When + _, err := ServerVersion(context.Background(), srv.Client(), srv.URL) + + // Then + if err == nil { + t.Error("err = nil, want an error") + } + }) +} From 1ece7b13d92c685e7da0ed266d28ec949d991867 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:01:47 +0100 Subject: [PATCH 03/12] fix(cmd): name the credential variable that is actually read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Off api.flagsmith.com the unscoped credential variables are ignored by design, so every hint naming one told self-hosted users to set something that would have no effect — including the "not logged in" hint they reach by following the previous one. The variable name depends on the instance, so the hints that carry one become functions of it. hintMasterKeyOrLogin names none and stays a const. beep boop --- internal/cmd/envcred.go | 15 +++++++++++++++ internal/cmd/envcred_test.go | 17 +++++++++++++++++ internal/cmd/errors.go | 23 +++++++++++++++++------ internal/cmd/errors_test.go | 34 +++++++++++++++++++++++++++++++--- internal/cmd/init.go | 5 +++-- internal/cmd/login.go | 4 ++-- 6 files changed, 85 insertions(+), 13 deletions(-) diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 0589dbe..9ce177a 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -63,6 +63,21 @@ func envCredential(base, rawURL, defaultURL string) (name, value string) { return "", "" } +// envVarFor names the variable a user should set to reach an instance — the +// one envCredential will actually read, which off the default host is only ever +// the host-scoped form. +func envVarFor(base, rawURL, defaultURL string) string { + if urlHost(rawURL) == urlHost(defaultURL) { + return base + } + return scopedEnvName(base, rawURL) +} + +// apiKeyVar and accessTokenVar name the Admin API credential variables for the +// instance this invocation is talking to. +func apiKeyVar() string { return envVarFor(envAPIKey, apiURL, defaultAPIURL) } +func accessTokenVar() string { return envVarFor(envAccessToken, apiURL, defaultAPIURL) } + // lookupEnvFold finds an environment variable by case-insensitive name, // returning the name as actually set. Host-scoped variable names embed a // hostname, which is itself case-insensitive. diff --git a/internal/cmd/envcred_test.go b/internal/cmd/envcred_test.go index cb7e1e4..6d00032 100644 --- a/internal/cmd/envcred_test.go +++ b/internal/cmd/envcred_test.go @@ -23,6 +23,23 @@ func TestScopedEnvName(t *testing.T) { } } +// The variable a user should set is the one that will actually be read: the +// unscoped form only where it is trusted, the host-scoped form everywhere else. +func TestEnvVarFor(t *testing.T) { + cases := map[string]string{ + "https://api.flagsmith.com": envAPIKey, + "https://api.flagsmith.com/": envAPIKey, + "https://API.Flagsmith.com": envAPIKey, + "https://flagsmith.example.com": "FLAGSMITH_API_KEY_flagsmith_example_com", + "http://localhost:8000": "FLAGSMITH_API_KEY_localhost_8000", + } + for in, want := range cases { + if got := envVarFor(envAPIKey, in, defaultAPIURL); got != want { + t.Errorf("envVarFor(%q) = %q, want %q", in, got, want) + } + } +} + // Boolean switches read a value, not merely presence: FLAGSMITH_NO_INPUT=false // must not disable prompting. func TestEnvBool(t *testing.T) { diff --git a/internal/cmd/errors.go b/internal/cmd/errors.go index 330a27c..1c1d72e 100644 --- a/internal/cmd/errors.go +++ b/internal/cmd/errors.go @@ -19,12 +19,9 @@ import ( const ( hintPricing = "This isn't available on your current plan — see https://flagsmith.com/pricing" hintQuota = "Enterprise plans can raise this limit — get in touch: https://docs.flagsmith.com/support#getting-in-touch" - hintLogin = "Run `flagsmith login`, or set FLAGSMITH_API_KEY for non-interactive use." - hintMasterKey = "Set FLAGSMITH_API_KEY to use a Master API key instead." hintMasterKeyOrLogin = "Use a Master API key, or run `flagsmith login`." hintRelogin = "Run `flagsmith login` to re-authenticate." - hintAccessToken = "For an OAuth access token, set FLAGSMITH_ACCESS_TOKEN instead." hintServerSideKey = "Server-side keys are secrets — provide them via FLAGSMITH_ENVIRONMENT_KEY instead." hintEnvironmentKey = "Check FLAGSMITH_ENVIRONMENT_KEY, or the environment name/key passed with -e." @@ -43,6 +40,20 @@ const ( hintReportIssue = "Think this shouldn't happen? Tell us: https://github.com/Flagsmith/flagsmith-cli/issues/new" ) +// Hints that name a credential variable are functions of the instance in play, +// because the name is: the unscoped form is read only for the default host. +func hintLogin() string { + return fmt.Sprintf("Run `flagsmith login`, or set %s for non-interactive use.", apiKeyVar()) +} + +func hintMasterKey() string { + return fmt.Sprintf("Set %s to use a Master API key instead.", apiKeyVar()) +} + +func hintAccessToken() string { + return fmt.Sprintf("For an OAuth access token, set %s instead.", accessTokenVar()) +} + // docsHint points at a page under docs.flagsmith.com. func docsHint(path string) string { return "See https://docs.flagsmith.com/" + path @@ -81,17 +92,17 @@ func hintFor(err error) string { } switch { case errors.Is(err, auth.ErrNotLoggedIn): - return hintLogin + return hintLogin() case errors.Is(err, auth.ErrNoDiscovery): return hintAPIURL case errors.Is(err, auth.ErrRefreshFailed): return hintRelogin case errors.Is(err, auth.ErrKeychainUnavailable): - return hintMasterKey + return hintMasterKey() case errors.Is(err, auth.ErrLegacyAuthtoken): return hintMasterKeyOrLogin case errors.Is(err, auth.ErrNotMasterKey): - return hintAccessToken + return hintAccessToken() case errors.Is(err, auth.ErrServerSideKey), errors.Is(err, config.ErrServerSideKey): return hintServerSideKey case errors.Is(err, api.ErrQuotaExceeded): diff --git a/internal/cmd/errors_test.go b/internal/cmd/errors_test.go index b7c3587..bc16648 100644 --- a/internal/cmd/errors_test.go +++ b/internal/cmd/errors_test.go @@ -21,7 +21,7 @@ func TestHintFor(t *testing.T) { err error want string }{ - {"not logged in", auth.ErrNotLoggedIn, hintLogin}, + {"not logged in", auth.ErrNotLoggedIn, hintLogin()}, {"no discovery document", auth.ErrNoDiscovery, hintAPIURL}, {"no discovery document wrapped", fmt.Errorf("%s returned 404: %w", "https://x/.well-known", auth.ErrNoDiscovery), hintAPIURL}, {"plan gated", api.ErrPlanGated, hintPricing}, @@ -29,11 +29,11 @@ func TestHintFor(t *testing.T) { {"quota exceeded", api.ErrQuotaExceeded, hintQuota}, {"quota exceeded wrapped", fmt.Errorf("create segment: %w", api.ErrQuotaExceeded), hintQuota}, {"workflow gated", api.ErrWorkflowGated, docsHint("advanced-use/change-requests")}, - {"keychain unavailable", auth.ErrKeychainUnavailable, hintMasterKey}, + {"keychain unavailable", auth.ErrKeychainUnavailable, hintMasterKey()}, {"session refresh failed wrapped", fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom")), hintRelogin}, {"server-side key in FLAGSMITH_API_KEY", auth.ErrServerSideKey, hintServerSideKey}, {"legacy authtoken", auth.ErrLegacyAuthtoken, hintMasterKeyOrLogin}, - {"not a master key", auth.ErrNotMasterKey, hintAccessToken}, + {"not a master key", auth.ErrNotMasterKey, hintAccessToken()}, {"server-side key in config file", fmt.Errorf("flagsmith.json: %w", config.ErrServerSideKey), hintServerSideKey}, {"marked unexpected", bug.Mark(errors.New("boom")), hintReportIssue}, {"specific hint beats report-issue", bug.Mark(fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom"))), hintRelogin}, @@ -51,6 +51,34 @@ func TestHintFor(t *testing.T) { } } +// Unscoped credential variables are read only for the default host, so a hint +// that names one off it sends a self-hosted user to set something that will be +// ignored. +func TestCredentialHintsNameTheVariableThatIsRead(t *testing.T) { + defer func(u string) { apiURL = u }(apiURL) + + apiURL = "https://flagsmith.example.com" + scoped := map[string]string{ + "hintLogin": hintLogin(), + "hintMasterKey": hintMasterKey(), + "hintAccessToken": hintAccessToken(), + } + for name, got := range scoped { + want := "FLAGSMITH_API_KEY_flagsmith_example_com" + if name == "hintAccessToken" { + want = "FLAGSMITH_ACCESS_TOKEN_flagsmith_example_com" + } + if !strings.Contains(got, want) { + t.Errorf("%s() = %q, want it to name %s", name, got, want) + } + } + + apiURL = defaultAPIURL + if got := hintLogin(); !strings.Contains(got, envAPIKey+" ") { + t.Errorf("hintLogin() on the default host = %q, want the unscoped variable", got) + } +} + func TestReportError(t *testing.T) { newCmd := func(buf *bytes.Buffer) *cobra.Command { c := &cobra.Command{Use: "demo", Short: "demo"} diff --git a/internal/cmd/init.go b/internal/cmd/init.go index a094690..72b045c 100644 --- a/internal/cmd/init.go +++ b/internal/cmd/init.go @@ -137,8 +137,9 @@ func runInit(cmd *cobra.Command, args []string) error { cred, err := resolveCredential(ctx) if errors.Is(err, auth.ErrNotLoggedIn) { if !interactive() { - return withHint(errors.New("no credentials found, and a browser login needs a TTY"), - "Set FLAGSMITH_API_KEY, run in a CI OIDC context with an org trust relationship, or run `flagsmith login` interactively first.") + return hintf(errors.New("no credentials found, and a browser login needs a TTY"), + "Set %s, run in a CI OIDC context with an org trust relationship, or run `flagsmith login` interactively first.", + apiKeyVar()) } if err := browserLogin(cmd); err != nil { return err diff --git a/internal/cmd/login.go b/internal/cmd/login.go index c383a5a..2984b04 100644 --- a/internal/cmd/login.go +++ b/internal/cmd/login.go @@ -43,13 +43,13 @@ func browserLogin(cmd *cobra.Command) error { // authorization, not a liveness switch, so it does not block login.) if noInput() { return withHint(errors.New("browser login needs a terminal and cannot run with --no-input"), - hintMasterKey) + hintMasterKey()) } // The session lives in the OS keychain; without one, minting tokens we // can't store would strand a live session — fail closed toward the env var. if !auth.KeychainAvailable() { return withHint(errors.New("no OS keychain available to store the session"), - hintMasterKey) + hintMasterKey()) } open := browser.OpenURL if noBrowser || !stdinIsTTY() { From 7639566c188e4cd05f7475ef7fe71f91294b2f0b Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:03:57 +0100 Subject: [PATCH 04/12] fix(cmd): nudge to a Master API key on a pre-OAuth instance `flagsmith init` and `flagsmith login` against a Flagsmith older than 2.255.0 dead-ended on a 404 from the discovery endpoint, with no mention of the Master API key that would have worked. Probe /version only once discovery has already failed, so the happy path costs nothing. An image tag that is not a version says nothing about age, so anything unparseable keeps the generic hint rather than sending someone with a healthy instance chasing an upgrade. Anyone reaching this is self-hosted by definition, so the key variable is named host-scoped. beep boop --- go.mod | 2 +- internal/cmd/cmd_test.go | 83 ++++++++++++++++++++++++++++++++++++++++ internal/cmd/login.go | 30 +++++++++++++++ 3 files changed, 114 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index a7615ad..d3229a8 100644 --- a/go.mod +++ b/go.mod @@ -4,6 +4,7 @@ go 1.26 require ( github.com/Flagsmith/flagsmith-go-client/v5 v5.1.0 + github.com/blang/semver/v4 v4.0.0 github.com/charmbracelet/huh v1.0.0 github.com/fatih/color v1.19.0 github.com/itchyny/gojq v0.12.19 @@ -18,7 +19,6 @@ require ( require ( github.com/atotto/clipboard v0.1.4 // indirect github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect - github.com/blang/semver/v4 v4.0.0 // indirect github.com/catppuccin/go v0.3.0 // indirect github.com/charmbracelet/bubbles v0.21.1-0.20250623103423-23b8fd6302d7 // indirect github.com/charmbracelet/bubbletea v1.3.6 // indirect diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 0b98cd2..76656b0 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -7607,6 +7608,88 @@ func TestLoginFailsClosedWithoutKeychain(t *testing.T) { } } +// oldInstance serves a /version/ document reporting tag, and nothing else — in +// particular no OAuth metadata, like a Flagsmith predating the browser login. +// An empty tag serves no version endpoint at all. +func oldInstance(t *testing.T, tag string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + if tag != "" { + mux.HandleFunc("GET /version/", func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, `{"image_tag":%q}`, tag) + }) + } + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv +} + +// A Flagsmith too old to serve OAuth metadata has a way forward — a Master API +// key — that the bare 404 from the discovery endpoint does not mention. +func TestLoginAgainstAnInstanceWithoutOAuth(t *testing.T) { + t.Run("old version names the version and the key variable to set", func(t *testing.T) { + // Given + isolateStorage(t) + srv := oldInstance(t, "2.180.3") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + hint := hintFor(err) + if err == nil { + t.Fatal("err = nil, want a login failure") + } + for _, want := range []string{"2.180.3", minOAuthVersion, scopedEnvName(envAPIKey, srv.URL)} { + if !strings.Contains(hint, want) { + t.Errorf("hint = %q, want it to mention %q", hint, want) + } + } + }) + + t.Run("unparseable image tag falls back to the generic hint", func(t *testing.T) { + // Given: self-hosted images are routinely tagged like this + isolateStorage(t) + srv := oldInstance(t, "latest") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) + + t.Run("no version endpoint falls back to the generic hint", func(t *testing.T) { + // Given + isolateStorage(t) + srv := oldInstance(t, "") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) + + t.Run("current version falls back to the generic hint", func(t *testing.T) { + // Given: new enough for OAuth, so a missing document is not about age + isolateStorage(t) + srv := oldInstance(t, "2.262.0") + + // When + _, err := run("", "login", "--api-url", srv.URL, "--no-browser") + + // Then + if got := hintFor(err); got != hintAPIURL { + t.Errorf("hint = %q, want the generic %q", got, hintAPIURL) + } + }) +} + func TestRefreshPersistsToKeychain(t *testing.T) { // Given isolateStorage(t) diff --git a/internal/cmd/login.go b/internal/cmd/login.go index 2984b04..0bcbbc5 100644 --- a/internal/cmd/login.go +++ b/internal/cmd/login.go @@ -1,9 +1,11 @@ package cmd import ( + "context" "errors" "fmt" + "github.com/blang/semver/v4" "github.com/pkg/browser" "github.com/spf13/cobra" @@ -12,6 +14,10 @@ import ( "github.com/Flagsmith/flagsmith-cli/v2/internal/output" ) +// minOAuthVersion is the Flagsmith release that added the admin-api OAuth +// scope the CLI logs in with: https://github.com/Flagsmith/flagsmith/releases/tag/v2.255.0 +const minOAuthVersion = "2.255.0" + var noBrowser bool func newLoginCmd() *cobra.Command { @@ -37,6 +43,27 @@ func newLoginCmd() *cobra.Command { return cmd } +// tooOldForOAuthHint explains a missing discovery document when the instance +// turns out to be a Flagsmith predating the OAuth login, and reports "" — no +// hint, leaving the generic one — whenever that cannot be established. Only a +// self-hosted instance can be this old, so the Master API key it points at is +// named host-scoped. An instance reporting an image tag that is not a version +// ("latest", a commit sha) says nothing about its age: claiming it is too old +// would send a user with a healthy instance chasing an upgrade they don't need. +func tooOldForOAuthHint(ctx context.Context) string { + tag, err := api.ServerVersion(ctx, sharedHTTPClient(), apiURL) + if err != nil { + return "" + } + got, err := semver.ParseTolerant(tag) + if err != nil || got.GTE(semver.MustParse(minOAuthVersion)) { + return "" + } + return fmt.Sprintf( + "This instance runs Flagsmith %s; browser login needs %s or newer. Upgrade it, or set %s to a Master API key.", + tag, minOAuthVersion, apiKeyVar()) +} + func browserLogin(cmd *cobra.Command) error { // --no-input promises zero interaction; a browser login is nothing but // interaction. Master API keys go through FLAGSMITH_API_KEY. (--yes is @@ -57,6 +84,9 @@ func browserLogin(cmd *cobra.Command) error { } creds, err := auth.Login(cmd.Context(), sharedHTTPClient(), apiURL, open, cmd.OutOrStdout()) if err != nil { + if errors.Is(err, auth.ErrNoDiscovery) { + return withHint(err, tooOldForOAuthHint(cmd.Context())) + } return err } if err := auth.Save(creds); err != nil { From 7d8a2da7b1bd73424d7a03c3a0894ccf98b39e54 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:06:00 +0100 Subject: [PATCH 05/12] fix(cmd): report an unscoped credential as ignored, not as no credential MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A self-hosted user who exported FLAGSMITH_API_KEY was told "not logged in" and hinted to set FLAGSMITH_API_KEY — the thing they had just done. Their key was fine; only its name was, and the CLI knew that and didn't say so. Say which variable is being ignored and which one to set instead. This no longer wraps ErrNotLoggedIn, so `init` stops offering a browser login over a credential the user explicitly set: silently discarding it is the bug. The redirected-host tests now assert on the withholding itself rather than the sentinel that used to stand in for it. beep boop --- internal/cmd/auth.go | 10 +++++++++ internal/cmd/cmd_test.go | 48 ++++++++++++++++++++++++++++++++++++---- internal/cmd/envcred.go | 16 ++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index 91b2290..a5ee88d 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -2,6 +2,7 @@ package cmd import ( "context" + "errors" "fmt" "io" "strconv" @@ -100,6 +101,15 @@ func loadCredential(ctx context.Context) (*activeCredential, error) { } creds, err := auth.Load(apiURL) + if errors.Is(err, auth.ErrNotLoggedIn) { + // Reporting "not logged in" to someone who did set a credential sends + // them to do what they just did. Their key is fine; its name isn't. + if set, use := ignoredUnscopedCredential(); set != "" { + return nil, hintf(fmt.Errorf("%s is set, but ignored for %s", set, apiURL), + "Credentials are host-scoped away from %s — set %s instead.", + urlHost(defaultAPIURL), use) + } + } if err != nil { return nil, err } diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 76656b0..91dd730 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -7385,8 +7385,8 @@ func TestUnscopedCredentialNotSentToRedirectedHost(t *testing.T) { _, err := run("", "flag", "list") // Then - if !errors.Is(err, auth.ErrNotLoggedIn) { - t.Errorf("err = %v, want ErrNotLoggedIn (credential withheld)", err) + if err == nil || !strings.Contains(err.Error(), envAPIKey) { + t.Errorf("err = %v, want the credential withheld and said to be", err) } if got := f.featuresCalls(); got != 0 { t.Errorf("features calls = %d, want 0 — no request should carry the key", got) @@ -7424,8 +7424,8 @@ func TestUnscopedAccessTokenNotSentToRedirectedHost(t *testing.T) { _, err := run("", "auth", "status") // Then - if !errors.Is(err, auth.ErrNotLoggedIn) { - t.Errorf("err = %v, want ErrNotLoggedIn (bearer withheld)", err) + if err == nil || !strings.Contains(err.Error(), envAccessToken) { + t.Errorf("err = %v, want the bearer withheld and said to be", err) } if got := f.organisationLists(); got != 0 { t.Errorf("organisation calls = %d, want 0 — no request should carry the bearer", got) @@ -7587,6 +7587,46 @@ func TestEnvBeatsKeychain(t *testing.T) { } } +// A credential set in the unscoped variable is ignored off the default host. +// Reporting that as "not logged in" sends the user to do what they just did. +func TestUnscopedCredentialIsReportedAsIgnored(t *testing.T) { + t.Run("names both the ignored variable and the one to set", func(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + t.Setenv(envAPIKey, masterKey) + + // When + _, err := run("", "project", "list", "--api-url", f.srv.URL) + + // Then + if err == nil { + t.Fatal("err = nil, want the ignored credential reported") + } + if !strings.Contains(err.Error(), envAPIKey) || strings.Contains(err.Error(), "not logged in") { + t.Errorf("err = %v, want it to name the ignored variable rather than report a login", err) + } + if want := scopedEnvName(envAPIKey, f.srv.URL); !strings.Contains(hintFor(err), want) { + t.Errorf("hint = %q, want it to name %s", hintFor(err), want) + } + }) + + t.Run("silent when the variable is scoped to this instance", func(t *testing.T) { + // Given + isolateStorage(t) + f := newFakeInstance(t) + setMasterKey(t, f.srv.URL) + + // When + _, err := run("", "project", "list", "--api-url", f.srv.URL) + + // Then + if err != nil { + t.Fatalf("project list: %v", err) + } + }) +} + func TestLoginFailsClosedWithoutKeychain(t *testing.T) { // Given isolateStorage(t) diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 9ce177a..76ef31c 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -73,6 +73,22 @@ func envVarFor(base, rawURL, defaultURL string) string { return scopedEnvName(base, rawURL) } +// ignoredUnscopedCredential names an unscoped credential variable that is set +// but cannot be read for this instance, together with the variable that would +// be. Both are "" when nothing is being ignored. It is only meaningful once +// scoped lookups have missed, which is the only place it is called from. +func ignoredUnscopedCredential() (set, use string) { + if urlHost(apiURL) == urlHost(defaultAPIURL) { + return "", "" + } + for _, base := range []string{envAPIKey, envAccessToken} { + if os.Getenv(base) != "" { + return base, scopedEnvName(base, apiURL) + } + } + return "", "" +} + // apiKeyVar and accessTokenVar name the Admin API credential variables for the // instance this invocation is talking to. func apiKeyVar() string { return envVarFor(envAPIKey, apiURL, defaultAPIURL) } From 166ccb67af9b5f1d7162c59b57d87d7a06d6d205 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:06:13 +0100 Subject: [PATCH 06/12] docs: document the host-scoped credential variables The scoping rule was only discoverable by hitting it, which is how it was reported. beep boop --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 3755395..7fd845f 100644 --- a/README.md +++ b/README.md @@ -88,5 +88,5 @@ flagsmith flag list # list the flags in the current environment ## Conventions - `--json` (or `FLAGSMITH_JSON_OUTPUT`) for machine-readable output; `--jq ` to filter it. -- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). +- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). Must be host-scoped against a self-hosted instance: `FLAGSMITH_API_KEY_flagsmith_example_com` for `https://flagsmith.example.com`. - Self-hosted: `--api-url` or `FLAGSMITH_API_URL`. From a4a53d849e2d5c893400eadfd1021f96a16c45c9 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:12:16 +0100 Subject: [PATCH 07/12] fix(cmd): scope the SDK credential hints to the SDK surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FLAGSMITH_ENVIRONMENT_KEY is host-scoped like the Admin variables, but off its own surface — sdkApiUrl, which defaults to Edge and so is usually not even the same host as the API. The hints naming it had the same defect as the Admin ones: correct on SaaS, useless anywhere else. The ser.* context guard needed the SDK URL to name the variable, and ran before that URL was resolved, so it moves below the resolution. Both surface URLs are now published as soon as resolveContext knows them rather than on the way out of applyContext, which is also what lets the guard name them at all. beep boop --- internal/cmd/cmd_test.go | 17 +++++++++++++++++ internal/cmd/context.go | 21 ++++++++++++++------- internal/cmd/envcred.go | 9 ++++++--- internal/cmd/errors.go | 16 +++++++++++----- internal/cmd/errors_test.go | 21 ++++++++++++++++++--- internal/cmd/evaluate.go | 2 +- internal/cmd/resolve.go | 11 ++++++----- internal/cmd/root.go | 8 ++++++-- 8 files changed, 79 insertions(+), 26 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 91dd730..468a92f 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -2160,6 +2160,23 @@ func TestConfigCommand(t *testing.T) { t.Errorf("err = %v (hint %q), want a hint pointing at FLAGSMITH_ENVIRONMENT_KEY", err, hintFor(err)) } }) + + // The SDK credential is scoped to the SDK surface, so the variable to set + // is only knowable once that URL is resolved. + t.Run("server-side key rejection names the scoped variable", func(t *testing.T) { + // Given + isolateStorage(t) + tempRepo(t) + + // When + _, err := run("", "config", "-e", "ser.AbCd", "--sdk-api-url", "https://sdk.example.com") + + // Then + want := "FLAGSMITH_ENVIRONMENT_KEY_sdk_example_com" + if err == nil || !strings.Contains(hintFor(err), want) { + t.Errorf("err = %v (hint %q), want a hint pointing at %s", err, hintFor(err), want) + } + }) } // fakeTTY makes prompts believe stdin is a terminal for one test. diff --git a/internal/cmd/context.go b/internal/cmd/context.go index 949b0ca..79c8c9d 100644 --- a/internal/cmd/context.go +++ b/internal/cmd/context.go @@ -143,14 +143,10 @@ func resolveContext(cmd *cobra.Command) (*projectContext, error) { return file.Organisation.Value(), true }, nil) - // environment (client-side key; ser.* never belongs in context) + // environment (a client-side key; the ser.* guard is below, once the SDK + // surface is known and the variable to name with it) pc.Environment = contextValue(cmd, "environment", environmentFlag, "FLAGSMITH_ENVIRONMENT", asString, func() (any, bool) { return file.Environment, file.Environment != "" }, nil) - if key, ok := pc.Environment.Value.(string); ok && strings.HasPrefix(key, "ser.") { - return nil, withHint( - errors.New("the environment context takes a client-side key"), - hintServerSideKey) - } pc.APIURL = contextValue(cmd, "api-url", apiURLFlag, "FLAGSMITH_API_URL", trimSlash, func() (any, bool) { return strings.TrimRight(file.APIURL, "/"), file.APIURL != "" }, defaultAPIURL) @@ -163,6 +159,18 @@ func resolveContext(cmd *cobra.Command) (*projectContext, error) { pc.SDKAPIURL = contextValue(cmd, "sdk-api-url", sdkAPIURLFlag, "FLAGSMITH_SDK_API_URL", trimSlash, func() (any, bool) { return strings.TrimRight(file.SDKAPIURL, "/"), file.SDKAPIURL != "" }, sdkDefault) + // Both surfaces are now known; the credential layer and every hint naming a + // host-scoped variable read them from here. + apiURL = pc.apiURL() + sdkAPIURL, _ = pc.SDKAPIURL.Value.(string) + + // ser.* is a secret and never a context value. + if key, ok := pc.Environment.Value.(string); ok && strings.HasPrefix(key, "ser.") { + return nil, withHint( + errors.New("the environment context takes a client-side key"), + hintServerSideKey()) + } + // Cosmetic name enrichment from the local cache — never the network. names := cache.Load(pc.apiURL()) if id, ok := pc.Project.Value.(int); ok { @@ -187,6 +195,5 @@ func applyContext(cmd *cobra.Command) (*projectContext, error) { for _, w := range pc.Warnings { fmt.Fprintf(cmd.ErrOrStderr(), "Warning: %s\n", w) } - apiURL = pc.apiURL() return pc, nil } diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 76ef31c..4403ffb 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -90,9 +90,12 @@ func ignoredUnscopedCredential() (set, use string) { } // apiKeyVar and accessTokenVar name the Admin API credential variables for the -// instance this invocation is talking to. -func apiKeyVar() string { return envVarFor(envAPIKey, apiURL, defaultAPIURL) } -func accessTokenVar() string { return envVarFor(envAccessToken, apiURL, defaultAPIURL) } +// instance this invocation is talking to; environmentKeyVar names the SDK one, +// which scopes to the SDK surface — a host of its own, and by default not even +// the same one. +func apiKeyVar() string { return envVarFor(envAPIKey, apiURL, defaultAPIURL) } +func accessTokenVar() string { return envVarFor(envAccessToken, apiURL, defaultAPIURL) } +func environmentKeyVar() string { return envVarFor(envEnvironmentKey, sdkAPIURL, defaultSDKAPIURL) } // lookupEnvFold finds an environment variable by case-insensitive name, // returning the name as actually set. Host-scoped variable names embed a diff --git a/internal/cmd/errors.go b/internal/cmd/errors.go index 1c1d72e..8256186 100644 --- a/internal/cmd/errors.go +++ b/internal/cmd/errors.go @@ -22,11 +22,9 @@ const ( hintMasterKeyOrLogin = "Use a Master API key, or run `flagsmith login`." hintRelogin = "Run `flagsmith login` to re-authenticate." - hintServerSideKey = "Server-side keys are secrets — provide them via FLAGSMITH_ENVIRONMENT_KEY instead." - hintEnvironmentKey = "Check FLAGSMITH_ENVIRONMENT_KEY, or the environment name/key passed with -e." - hintSDKAPIURL = "Check --sdk-api-url (or `sdkApiUrl`) — it must point at a Flagsmith SDK API." - hintAPIURL = "Check --api-url (or `apiUrl`) — it must point at a Flagsmith API." + hintSDKAPIURL = "Check --sdk-api-url (or `sdkApiUrl`) — it must point at a Flagsmith SDK API." + hintAPIURL = "Check --api-url (or `apiUrl`) — it must point at a Flagsmith API." hintEnvironmentList = "Run `flagsmith environment list` to see the environments in this project." hintProjectList = "Run `flagsmith project list` to see the projects you can access." @@ -54,6 +52,14 @@ func hintAccessToken() string { return fmt.Sprintf("For an OAuth access token, set %s instead.", accessTokenVar()) } +func hintServerSideKey() string { + return fmt.Sprintf("Server-side keys are secrets — provide them via %s instead.", environmentKeyVar()) +} + +func hintEnvironmentKey() string { + return fmt.Sprintf("Check %s, or the environment name/key passed with -e.", environmentKeyVar()) +} + // docsHint points at a page under docs.flagsmith.com. func docsHint(path string) string { return "See https://docs.flagsmith.com/" + path @@ -104,7 +110,7 @@ func hintFor(err error) string { case errors.Is(err, auth.ErrNotMasterKey): return hintAccessToken() case errors.Is(err, auth.ErrServerSideKey), errors.Is(err, config.ErrServerSideKey): - return hintServerSideKey + return hintServerSideKey() case errors.Is(err, api.ErrQuotaExceeded): return hintQuota case errors.Is(err, api.ErrPlanGated): diff --git a/internal/cmd/errors_test.go b/internal/cmd/errors_test.go index bc16648..860140c 100644 --- a/internal/cmd/errors_test.go +++ b/internal/cmd/errors_test.go @@ -31,10 +31,10 @@ func TestHintFor(t *testing.T) { {"workflow gated", api.ErrWorkflowGated, docsHint("advanced-use/change-requests")}, {"keychain unavailable", auth.ErrKeychainUnavailable, hintMasterKey()}, {"session refresh failed wrapped", fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom")), hintRelogin}, - {"server-side key in FLAGSMITH_API_KEY", auth.ErrServerSideKey, hintServerSideKey}, + {"server-side key in FLAGSMITH_API_KEY", auth.ErrServerSideKey, hintServerSideKey()}, {"legacy authtoken", auth.ErrLegacyAuthtoken, hintMasterKeyOrLogin}, {"not a master key", auth.ErrNotMasterKey, hintAccessToken()}, - {"server-side key in config file", fmt.Errorf("flagsmith.json: %w", config.ErrServerSideKey), hintServerSideKey}, + {"server-side key in config file", fmt.Errorf("flagsmith.json: %w", config.ErrServerSideKey), hintServerSideKey()}, {"marked unexpected", bug.Mark(errors.New("boom")), hintReportIssue}, {"specific hint beats report-issue", bug.Mark(fmt.Errorf("%w: %w", auth.ErrRefreshFailed, errors.New("boom"))), hintRelogin}, {"explicit hint wins over automatic", withHint(api.ErrPlanGated, "custom"), "custom"}, @@ -55,7 +55,22 @@ func TestHintFor(t *testing.T) { // that names one off it sends a self-hosted user to set something that will be // ignored. func TestCredentialHintsNameTheVariableThatIsRead(t *testing.T) { - defer func(u string) { apiURL = u }(apiURL) + defer func(a, s string) { apiURL, sdkAPIURL = a, s }(apiURL, sdkAPIURL) + + // The SDK credential scopes to the SDK surface, which is its own host. + sdkAPIURL = "https://sdk.example.com" + for name, got := range map[string]string{ + "hintServerSideKey": hintServerSideKey(), + "hintEnvironmentKey": hintEnvironmentKey(), + } { + if want := "FLAGSMITH_ENVIRONMENT_KEY_sdk_example_com"; !strings.Contains(got, want) { + t.Errorf("%s() = %q, want it to name %s", name, got, want) + } + } + sdkAPIURL = defaultSDKAPIURL + if got := hintEnvironmentKey(); !strings.Contains(got, envEnvironmentKey+",") { + t.Errorf("hintEnvironmentKey() on the default SDK host = %q, want the unscoped variable", got) + } apiURL = "https://flagsmith.example.com" scoped := map[string]string{ diff --git a/internal/cmd/evaluate.go b/internal/cmd/evaluate.go index 66b0d2f..887968d 100644 --- a/internal/cmd/evaluate.go +++ b/internal/cmd/evaluate.go @@ -304,7 +304,7 @@ func evalError(sdkURL string, err error) error { return bug.Mark(fmt.Errorf("evaluating flags on %s failed", sdkURL)) case http.StatusUnauthorized, http.StatusForbidden: return withHint(fmt.Errorf("%s rejected the environment key (%s)", sdkURL, apiErr.ResponseStatus), - hintEnvironmentKey) + hintEnvironmentKey()) case http.StatusNotFound: return withHint(fmt.Errorf("%s has no SDK API to evaluate against (%s)", sdkURL, apiErr.ResponseStatus), hintSDKAPIURL) diff --git a/internal/cmd/resolve.go b/internal/cmd/resolve.go index 00bc6db..363ec2a 100644 --- a/internal/cmd/resolve.go +++ b/internal/cmd/resolve.go @@ -82,13 +82,14 @@ func resolveEnvironment(cmd *cobra.Command, pc *projectContext, cred *activeCred // The SDK credential doubles as an environment reference, host-scoped // to the SDK surface. sdkURL, _ := pc.SDKAPIURL.Value.(string) - _, ref = envCredential(envEnvironmentKey, sdkURL, defaultSDKAPIURL) + name, r := envCredential(envEnvironmentKey, sdkURL, defaultSDKAPIURL) + ref = r // Server-side keys belong in that variable but can never resolve an // environment over the Admin API. Name the variable, never its value: // it is a secret and must stay out of stderr and CI logs. if strings.HasPrefix(ref, "ser.") { return api.Environment{}, withHint( - errors.New("FLAGSMITH_ENVIRONMENT_KEY holds a server-side key, which cannot identify an environment for Admin commands"), + fmt.Errorf("%s holds a server-side key, which cannot identify an environment for Admin commands", name), "Pass -e, set FLAGSMITH_ENVIRONMENT, or run `flagsmith init`.") } } @@ -126,8 +127,8 @@ func sdkEnvironmentKey(cmd *cobra.Command, pc *projectContext) (string, error) { } ref, _ := pc.Environment.Value.(string) if ref == "" { - return "", withHint(errors.New("no environment key"), - "Set FLAGSMITH_ENVIRONMENT_KEY, or pass -e.") + return "", hintf(errors.New("no environment key"), + "Set %s, or pass -e.", environmentKeyVar()) } names := cache.Load(pc.apiURL()).Environments if _, cached := names[ref]; cached { @@ -165,7 +166,7 @@ func resolveEnvironmentRef(cmd *cobra.Command, cred *activeCredential, projectID if strings.HasPrefix(ref, "ser.") { return nil, withHint( errors.New("the environment reference takes a client-side key, not a server-side one"), - hintServerSideKey) + hintServerSideKey()) } envs, err := cred.client().Environments(cmd.Context(), projectID) if err != nil { diff --git a/internal/cmd/root.go b/internal/cmd/root.go index b5501b1..82b6e14 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -76,8 +76,12 @@ func singleLineUsage(template string) string { return strings.Replace(template, defaultBlock, oneLine, 1) } -// apiURL is the resolved instance URL for the current invocation. -var apiURL string +// apiURL and sdkAPIURL are the resolved Admin and SDK surface URLs for the +// current invocation, set by resolveContext as soon as each is known. +var ( + apiURL string + sdkAPIURL string +) var ( apiURLFlag string From bf7edc935e608dab6f05bd5dca329a3efda2331f Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 09:14:22 +0100 Subject: [PATCH 08/12] fix(auth): let the caller name the variable a bad key came from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Master API key variable is host-scoped, so a sentinel that spells FLAGSMITH_API_KEY into its own message names the wrong variable for every self-hosted instance — telling a user their FLAGSMITH_API_KEY holds a server-side key when what they set was FLAGSMITH_API_KEY_flagsmith_example_com. The rejections become predicates and loadCredential supplies the subject: the name envCredential actually read the value from. This is how config.ErrServerSideKey already reads against its file path. beep boop --- internal/auth/kind.go | 15 +++++++++------ internal/cmd/auth.go | 2 +- internal/cmd/cmd_test.go | 5 +++++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/auth/kind.go b/internal/auth/kind.go index 07eb758..809168b 100644 --- a/internal/auth/kind.go +++ b/internal/auth/kind.go @@ -20,15 +20,18 @@ const ( var legacyAuthtokenPattern = regexp.MustCompile(`^[0-9a-fA-F]{40}$`) -// ValidateMasterKey's rejections. +// ValidateMasterKey's rejections. Each reads as the predicate of a sentence +// whose subject is the variable the value came from, which only the caller +// knows: the Master API key variable is host-scoped, so naming it here would +// name the wrong one for every self-hosted instance. var ( - ErrServerSideKey = errors.New("FLAGSMITH_API_KEY contains a server-side environment key") - ErrLegacyAuthtoken = errors.New("FLAGSMITH_API_KEY contains a legacy user authtoken, which is not supported") - ErrNotMasterKey = errors.New("FLAGSMITH_API_KEY is not a Master API key (expected {prefix}.{secret})") + ErrServerSideKey = errors.New("holds a server-side environment key") + ErrLegacyAuthtoken = errors.New("holds a legacy user authtoken, which is not supported") + ErrNotMasterKey = errors.New("is not a Master API key (expected {prefix}.{secret})") ) -// ValidateMasterKey checks that a FLAGSMITH_API_KEY value is a Master API key. -// Each Admin API env var maps to exactly one credential kind, so the scheme is +// ValidateMasterKey checks that a credential value is a Master API key. Each +// Admin API env var maps to exactly one credential kind, so the scheme is // never guessed from token shape; this only turns common paste-mistakes into // actionable errors instead of a silently rejected request. func ValidateMasterKey(value string) error { diff --git a/internal/cmd/auth.go b/internal/cmd/auth.go index a5ee88d..60d1eec 100644 --- a/internal/cmd/auth.go +++ b/internal/cmd/auth.go @@ -87,7 +87,7 @@ func resolveCredential(ctx context.Context) (*activeCredential, error) { func loadCredential(ctx context.Context) (*activeCredential, error) { if name, v := envCredential(envAPIKey, apiURL, defaultAPIURL); v != "" { if err := auth.ValidateMasterKey(v); err != nil { - return nil, err + return nil, fmt.Errorf("%s %w", name, err) } cred := &activeCredential{kind: auth.KindMaster, token: v, source: "$" + name, auth: api.APIKey(v)} cred.apiClient = newAPIClient(cred.auth) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index 468a92f..e42aebf 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -7577,6 +7577,11 @@ func TestEnvServerKeyRejected(t *testing.T) { if err == nil || !strings.Contains(hintFor(err), "FLAGSMITH_ENVIRONMENT_KEY") { t.Errorf("err = %v (hint %q), want a hint pointing at FLAGSMITH_ENVIRONMENT_KEY", err, hintFor(err)) } + // The rejection must name the variable the value was read from, which off + // the default host is never the unscoped one. + if want := scopedEnvName(envAPIKey, f.srv.URL); err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("err = %v, want it to name %s", err, want) + } } func TestEnvBeatsKeychain(t *testing.T) { From d65e144e5cb8ee1b18e4c6c4bc789645ff2cfed0 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 10:48:43 +0100 Subject: [PATCH 09/12] fix(auth): treat only a 404 as an absent discovery document Every non-200 was reported as "no authorization server metadata", so a rate-limited or briefly broken instance was described as one that has no OAuth support and hinted to check --api-url for a mistake that isn't there. A refusal that is not a 404 comes from a server that does have the document, and joins the other protocol surprises in this function as reportable. beep boop --- internal/auth/oauth.go | 8 +++++++- internal/auth/oauth_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/internal/auth/oauth.go b/internal/auth/oauth.go index 6c9cbf2..e52c7b4 100644 --- a/internal/auth/oauth.go +++ b/internal/auth/oauth.go @@ -142,9 +142,15 @@ func Discover(ctx context.Context, httpClient *http.Client, apiURL string) (*Met return nil, bug.Mark(fmt.Errorf("reaching %s: %w", u, err)) } defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { + // Only a 404 means the document is absent. Any other refusal comes from a + // server that has one, and saying otherwise would send a user with a + // rate-limited or briefly broken instance looking for a URL mistake. + if resp.StatusCode == http.StatusNotFound { return nil, fmt.Errorf("%s returned %s: %w", u, resp.Status, ErrNoDiscovery) } + if resp.StatusCode != http.StatusOK { + return nil, bug.Mark(fmt.Errorf("%s returned %s", u, resp.Status)) + } var md Metadata if err := json.NewDecoder(resp.Body).Decode(&md); err != nil { return nil, bug.Mark(fmt.Errorf("decoding authorization server metadata: %w", err)) diff --git a/internal/auth/oauth_test.go b/internal/auth/oauth_test.go index 8728ce8..0d566fe 100644 --- a/internal/auth/oauth_test.go +++ b/internal/auth/oauth_test.go @@ -17,6 +17,8 @@ import ( "sync" "testing" "time" + + "github.com/Flagsmith/flagsmith-cli/v2/internal/bug" ) // fakeAuthServer implements just enough of the Flagsmith OAuth 2.1 surface @@ -421,6 +423,29 @@ func TestDiscover(t *testing.T) { } }) + // Only a 404 says the document is absent. A server that is failing or + // refusing has one, and callers act on the difference. + t.Run("a failing endpoint is not an absent one", func(t *testing.T) { + for _, status := range []int{http.StatusUnauthorized, http.StatusTooManyRequests, http.StatusInternalServerError} { + // Given + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(status) + })) + + // When + _, err := Discover(context.Background(), http.DefaultClient, srv.URL) + srv.Close() + + // Then + if err == nil || errors.Is(err, ErrNoDiscovery) { + t.Errorf("%d: err = %v, want an error that is not ErrNoDiscovery", status, err) + } + if !errors.Is(err, bug.ErrUnexpected) { + t.Errorf("%d: err = %v, want it marked reportable", status, err) + } + } + }) + t.Run("invalid JSON", func(t *testing.T) { // Given srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From 50d3952cdd179365396d43d88e357b230c37357f Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 10:49:27 +0100 Subject: [PATCH 10/12] fix(cmd): keep IPv6 instances out of unexportable variable names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An instance addressed by IPv6 literal scoped to FLAGSMITH_API_KEY_[__1]_8000, which no POSIX shell can export — so the variable could be neither set nor found, and the hints added here would have asked for it by name. The brackets are URL syntax rather than part of the host, so they are dropped instead of encoded. This does cost injectivity: `::` and `-` both land on `__`. Nothing reverses these names, and a name that cannot be typed is worse than one that cannot be reversed. beep boop --- internal/cmd/envcred.go | 5 ++++- internal/cmd/envcred_test.go | 5 +++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 4403ffb..64f01ea 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -28,9 +28,12 @@ func envBool(name string) bool { // scopedEnvName is the host-scoped form of a credential variable for an // instance URL: the host and port with `-` written `__` and `.` and `:` -// written `_`. The scheme is not part of the scope. +// written `_`. The scheme is not part of the scope. The brackets around an +// IPv6 literal are dropped rather than encoded — they are URL syntax, and a +// name carrying them is one no shell can export. func scopedEnvName(base, rawURL string) string { host := urlHost(rawURL) + host = strings.NewReplacer("[", "", "]", "").Replace(host) host = strings.ReplaceAll(host, "-", "__") host = strings.ReplaceAll(host, ".", "_") host = strings.ReplaceAll(host, ":", "_") diff --git a/internal/cmd/envcred_test.go b/internal/cmd/envcred_test.go index 6d00032..9cfce12 100644 --- a/internal/cmd/envcred_test.go +++ b/internal/cmd/envcred_test.go @@ -15,6 +15,11 @@ func TestScopedEnvName(t *testing.T) { "http://localhost:8000": "FLAGSMITH_API_KEY_localhost_8000", "http://127.0.0.1:8000/api": "FLAGSMITH_API_KEY_127_0_0_1_8000", "https://Flagsmith.Example.COM": "FLAGSMITH_API_KEY_flagsmith_example_com", + // An IPv6 literal's brackets are URL syntax, and no shell can export a + // variable whose name carries them. + "http://[::1]:8000": "FLAGSMITH_API_KEY___1_8000", + "https://[2001:db8::1]:8443": "FLAGSMITH_API_KEY_2001_db8__1_8443", + "https://[2001:DB8::1]": "FLAGSMITH_API_KEY_2001_db8__1", } for in, want := range cases { if got := scopedEnvName(envAPIKey, in); got != want { From eeaee685e5f32720b0a78ba2b5936f0882ec35fd Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 12:48:34 +0100 Subject: [PATCH 11/12] fix(cmd): name a usable variable before an instance is resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A config file that fails to load aborts resolveContext before it publishes either surface URL, so a hint rendered from that error scoped its variable name to an empty host and asked for FLAGSMITH_ENVIRONMENT_KEY_ — a name that cannot be set and identifies no instance. Fall back to the unscoped name when no host is known. It is the right answer for the default host and the only answer available for a user whose instance URL was in the file that just failed to parse. Every test in the package shares the resolved-URL globals, so the one pinning this clears them: a real run is one command in its own process, which is the state that exposed this. beep boop --- internal/cmd/cmd_test.go | 21 +++++++++++++++++++++ internal/cmd/envcred.go | 7 +++++-- internal/cmd/envcred_test.go | 5 +++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go index e42aebf..0efde2c 100644 --- a/internal/cmd/cmd_test.go +++ b/internal/cmd/cmd_test.go @@ -2161,6 +2161,27 @@ func TestConfigCommand(t *testing.T) { } }) + // A bad config file fails resolveContext before it can publish either + // surface URL, so the hint has no instance to scope a name to. Every test + // in this process shares those globals, so clear them to model a real run, + // which is one command in a process of its own. + t.Run("a config that fails to load still names a usable variable", func(t *testing.T) { + // Given + isolateStorage(t) + writeConfig(t, tempRepo(t), `{"environment": "ser.SuperSecret123"}`) + defer func(a, s string) { apiURL, sdkAPIURL = a, s }(apiURL, sdkAPIURL) + apiURL, sdkAPIURL = "", "" + + // When + _, err := run("", "config") + + // Then + hint := hintFor(err) + if err == nil || !strings.Contains(hint, envEnvironmentKey+" ") { + t.Errorf("err = %v (hint %q), want the plain %s named", err, hint, envEnvironmentKey) + } + }) + // The SDK credential is scoped to the SDK surface, so the variable to set // is only knowable once that URL is resolved. t.Run("server-side key rejection names the scoped variable", func(t *testing.T) { diff --git a/internal/cmd/envcred.go b/internal/cmd/envcred.go index 64f01ea..5c2edc5 100644 --- a/internal/cmd/envcred.go +++ b/internal/cmd/envcred.go @@ -68,9 +68,12 @@ func envCredential(base, rawURL, defaultURL string) (name, value string) { // envVarFor names the variable a user should set to reach an instance — the // one envCredential will actually read, which off the default host is only ever -// the host-scoped form. +// the host-scoped form. An instance that is not resolved yet gets the unscoped +// name: a hint can be rendered from an error raised before the surface URLs are +// known, and a scope suffixed to nothing names nothing. func envVarFor(base, rawURL, defaultURL string) string { - if urlHost(rawURL) == urlHost(defaultURL) { + host := urlHost(rawURL) + if host == "" || host == urlHost(defaultURL) { return base } return scopedEnvName(base, rawURL) diff --git a/internal/cmd/envcred_test.go b/internal/cmd/envcred_test.go index 9cfce12..c6d6acb 100644 --- a/internal/cmd/envcred_test.go +++ b/internal/cmd/envcred_test.go @@ -37,6 +37,11 @@ func TestEnvVarFor(t *testing.T) { "https://API.Flagsmith.com": envAPIKey, "https://flagsmith.example.com": "FLAGSMITH_API_KEY_flagsmith_example_com", "http://localhost:8000": "FLAGSMITH_API_KEY_localhost_8000", + // No instance resolved yet: a scoped name would be a bare suffix that + // names nothing and cannot be set. + "": envAPIKey, + "/": envAPIKey, + "///": envAPIKey, } for in, want := range cases { if got := envVarFor(envAPIKey, in, defaultAPIURL); got != want { From ae5d42fc48fc0dbc149ec96e270e76f8d28a2af9 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Thu, 13 Aug 2026 12:56:10 +0100 Subject: [PATCH 12/12] docs: state the host-scoping rule instead of one example of it A single example of a host that needed no escaping left the reader to guess what happens to a hyphen, which is the one character whose encoding is not obvious. The example now carries a hyphen and a port, so it demonstrates every part of the rule it states. beep boop --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7fd845f..051e3aa 100644 --- a/README.md +++ b/README.md @@ -88,5 +88,5 @@ flagsmith flag list # list the flags in the current environment ## Conventions - `--json` (or `FLAGSMITH_JSON_OUTPUT`) for machine-readable output; `--jq ` to filter it. -- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). Must be host-scoped against a self-hosted instance: `FLAGSMITH_API_KEY_flagsmith_example_com` for `https://flagsmith.example.com`. +- Static credentials: `FLAGSMITH_API_KEY` (Admin API), `FLAGSMITH_ENVIRONMENT_KEY` (SDK). When self-hosting Flagsmith, append the host and port, doubling `-` and writing `.` and `:` as `_`: `https://flagsmith-staging.com:8000` reads `FLAGSMITH_API_KEY_flagsmith__staging_com_8000`. - Self-hosted: `--api-url` or `FLAGSMITH_API_URL`.