diff --git a/internal/cli/auth.go b/internal/cli/auth.go index c4237ebe..37b08093 100644 --- a/internal/cli/auth.go +++ b/internal/cli/auth.go @@ -411,7 +411,13 @@ func newAuthStatusCmd() *cobra.Command { prof := cfg.Current() p.Section("tracebloc auth") p.Field("status", "signed in") - p.Field("backend", cfg.CurrentEnv) + // sessionEnv, not the raw stored string: this line is the human-facing + // answer to "which backend am I on?", and it must be the same answer + // --check computes and the same one authedClient dials. Printing the + // stored value let `auth status` say `Dev` while every request went to + // dev — a status command that disagrees with the client is worse than + // no status command. + p.Field("backend", sessionEnv(cfg)) if prof.Email != "" { p.Field("account", prof.Email) } @@ -442,7 +448,7 @@ func newAuthStatusCmd() *cobra.Command { // (IsSilentError) so main() prints nothing. // // The target env is resolved exactly like `login` (--env, then $CLIENT_ENV, then -// prod), and must match the signed-in CurrentEnv — otherwise the probe would OK a +// prod), and must match the signed-in env as sessionEnv resolves it — otherwise the probe would OK a // stale session for the wrong backend and the installer would skip the very // `login` that switches env, provisioning into the wrong account (RFC-0001 §10). func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { @@ -454,18 +460,23 @@ func runAuthCheck(ctx context.Context, p *ui.Printer, envFlag string) error { return &exitError{code: exitFailure} } target := api.ResolveEnv(envFlag) - if !cfg.SignedIn() || cfg.CurrentEnv != target { + // Compare the RESOLVED session env, not the raw cfg.CurrentEnv: target comes + // out of api.ResolveEnv already normalised, so comparing it against the stored + // string made this the one place a `"current_env": "Dev"` config failed a probe + // for the session it is actually signed in to. + signedIn := sessionEnv(cfg) + if !cfg.SignedIn() || signedIn != target { if p.Verbose() { - if cfg.SignedIn() && cfg.CurrentEnv != target { - p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", cfg.CurrentEnv, target) + if cfg.SignedIn() && signedIn != target { + p.Hintf("Signed in to %q, but this run targets %q — run `tracebloc login`.", signedIn, target) } else { p.Hintf("Not signed in. Run `tracebloc login`.") } } return &exitError{code: exitFailure} } - // Signed in AND CurrentEnv == target: probe it. authedClient() builds the client - // for sessionEnv (== CurrentEnv == target) with the stored token — reuse it and + // Signed in AND the resolved session env == target: probe it. authedClient() + // builds the client for sessionEnv (== the value just compared) with the stored token — reuse it and // discard its message (the exit code is the contract here). client, _, err := authedClient() if err != nil { diff --git a/internal/cli/client.go b/internal/cli/client.go index d7246ee5..e5a3f830 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -143,12 +143,26 @@ func clientPrompter() prompter { } // sessionEnv resolves the backend env for the signed-in session: the env saved -// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. Shared -// by authedClient and logout so every authenticated call — including the revoke -// on sign-out — talks to the host the token was actually issued for. +// at login, falling back (legacy / empty config) to $CLIENT_ENV then prod. +// +// THE ONLY PLACE THAT DERIVES A SESSION ENV FROM A CONFIG. Every caller that +// wants "which backend is this signed-in session on?" — authedClient, logout's +// revoke, `cluster doctor`, `auth status --check`, the telemetry label — goes +// through here, so the answer cannot differ by caller. Reading cfg.CurrentEnv +// directly is the bug this function exists to prevent: it silently drops the +// $CLIENT_ENV fallback, and it skips the normalisation below. +// +// The result is normalised (trimmed, lower-cased) to match api.ResolveEnv, which +// lower-cases both its explicit argument and $CLIENT_ENV. Returning cfg.CurrentEnv +// verbatim made this the one env-resolving function in the CLI whose output was +// not normalised: harmless where the value only reaches api.BaseURL (which +// lower-cases again), but a false negative anywhere the value is COMPARED — a +// config carrying `"current_env": "Dev"` (migrateV1 stores a v1 `env` verbatim, +// and the file is hand-written in fixtures) failed `auth status --check --env dev` +// against a session that works perfectly. func sessionEnv(cfg *config.Config) string { - if cfg.CurrentEnv != "" { - return cfg.CurrentEnv + if e := strings.ToLower(strings.TrimSpace(cfg.CurrentEnv)); e != "" { + return e } return api.ResolveEnv("") } diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index cd3341a0..ef3edce7 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -111,7 +111,11 @@ func runClusterDoctor( // an error (5xx/403/decode) is a tracebloc-side problem, distinct from a // network failure to reach it at all. Conflating the two would blame the // user's network (and hand them a proxy remedy) for tracebloc's own error. - apiClient := newAPIClient(cfg.CurrentEnv) + // sessionEnv, not cfg.CurrentEnv: the session probe must target the same host + // authedClient would, or `doctor` reports on a backend no other command talks + // to. Reading CurrentEnv directly drops sessionEnv's $CLIENT_ENV fallback and + // its normalisation — a second resolution of the same question. + apiClient := newAPIClient(sessionEnv(cfg)) apiClient.Token = cfg.Current().Token if _, werr := apiClient.WhoAmI(ctx); werr != nil { var ae *api.APIError diff --git a/internal/cli/env_resolution_test.go b/internal/cli/env_resolution_test.go new file mode 100644 index 00000000..af879c7f --- /dev/null +++ b/internal/cli/env_resolution_test.go @@ -0,0 +1,482 @@ +package cli + +// The environment / base-URL resolution family (backend#2171). +// +// The CLI answers "which backend am I talking to?" in several places, and each +// recut of the release train produced one more finding about a site that answered +// it differently from its neighbour: cli#528 (the telemetry label resolved through +// ResolveEnv while the client used the config), #542 (a spool path resolved twice), +// #540 (the label and the sink resolved twice). The pattern is always the same — +// a SECOND resolution of a question already answered — so these tests pin the +// invariants rather than the individual sites: +// +// 1. sessionEnv is the ONLY function that turns a config into a session env, and +// it normalises (trim + lower-case) like api.ResolveEnv does. +// 2. Callers take the resolved value; they never re-derive it. +// +// NOT covered here, deliberately: api.BaseURL's unknown/empty -> prod fail-open. +// That behaviour is shared with the installer's `_backend_url` and diverges from +// client-runtime's controller.py (which refuses), so changing it is a three- +// component decision tracked on backend#2171, not a CLI-local cleanup. + +import ( + "errors" + "fmt" + "go/scanner" + "go/token" + "io/fs" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/tracebloc/cli/internal/api" + "github.com/tracebloc/cli/internal/cluster" + "github.com/tracebloc/cli/internal/config" + "github.com/tracebloc/cli/internal/telemetry" +) + +// --- 1. sessionEnv is the single, normalising resolution point ---------------- + +// TestSessionEnvNormalisesTheStoredEnv: sessionEnv used to return cfg.CurrentEnv +// verbatim, which made it the one env-resolving function in the CLI whose output +// was not normalised — api.ResolveEnv lower-cases both its argument and +// $CLIENT_ENV, api.BaseURL lower-cases, spoolEnvSlug trims and lower-cases. +// +// Verbatim is invisible where the value only reaches api.BaseURL (which +// lower-cases again) and load-bearing everywhere else: a value that is COMPARED +// (`auth status --check`) or TRIMMED by one consumer and not another (BaseURL does +// not trim; " dev " therefore fell through to PROD). +func TestSessionEnvNormalisesTheStoredEnv(t *testing.T) { + for _, tc := range []struct { + stored string + want string + }{ + {"dev", "dev"}, + {"Dev", "dev"}, // migrateV1 stores a v1 `env` verbatim + {" dev ", "dev"}, // hand-written / fixture config + {"PROD", "prod"}, + {"banana", "banana"}, // unknown is preserved, not coerced — see BaseURL note above + } { + t.Run(tc.stored, func(t *testing.T) { + t.Setenv("CLIENT_ENV", "") + cfg := &config.Config{CurrentEnv: tc.stored} + if got := sessionEnv(cfg); got != tc.want { + t.Fatalf("sessionEnv(current_env=%q) = %q, want %q — sessionEnv must "+ + "normalise like api.ResolveEnv, or its callers disagree about the "+ + "same session", tc.stored, got, tc.want) + } + }) + } +} + +// TestSessionEnvFallsBackToClientEnvOnlyWhenUnset pins the precedence the task +// brief calls load-bearing: config `current_env` BEATS $CLIENT_ENV, and +// $CLIENT_ENV is consulted only when `current_env` is absent. The offboard e2e +// fixture writes `"current_env": "prod"` into its config, so a change that let +// $CLIENT_ENV win would silently repoint that suite. +func TestSessionEnvFallsBackToClientEnvOnlyWhenUnset(t *testing.T) { + t.Setenv("CLIENT_ENV", "dev") + + if got := sessionEnv(&config.Config{CurrentEnv: "prod"}); got != api.EnvProd { + t.Fatalf("sessionEnv(current_env=prod) with CLIENT_ENV=dev = %q, want %q — "+ + "the signed-in env must beat $CLIENT_ENV", got, api.EnvProd) + } + if got := sessionEnv(&config.Config{}); got != api.EnvDev { + t.Fatalf("sessionEnv(no current_env) with CLIENT_ENV=dev = %q, want %q — "+ + "$CLIENT_ENV is the legacy/empty-config fallback", got, api.EnvDev) + } + t.Setenv("CLIENT_ENV", "") + if got := sessionEnv(&config.Config{}); got != api.EnvProd { + t.Fatalf("sessionEnv(no current_env, no CLIENT_ENV) = %q, want %q", got, api.EnvProd) + } +} + +// --- 2. no caller re-derives the session env ---------------------------------- + +// TestClusterDoctorProbesTheSessionEnv: `cluster doctor` built its API client +// from cfg.CurrentEnv directly instead of sessionEnv — a second answer to the +// question authedClient already answers. The whitespace case makes the two +// answers differ for real: api.BaseURL lower-cases but does NOT trim, so +// newAPIClient(" dev ") lands on the prod default while every other +// authenticated command talks to dev-api. +// +// A doctor that probes prod with a dev token reports "session expired" for a +// session that is fine — the worst possible output from a diagnostic. +func TestClusterDoctorProbesTheSessionEnv(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " dev ", Profiles: map[string]*config.Profile{ + " dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + var gotEnv string + orig := newAPIClient + newAPIClient = func(env string) *api.Client { + gotEnv = env + // No BaseURL: the WhoAmI below must fail locally rather than reach any + // real host. What is under test is the env, not the probe's verdict. + return &api.Client{HTTP: &http.Client{Timeout: time.Millisecond}} + } + t.Cleanup(func() { newAPIClient = orig }) + + // HERMETIC BY CONSTRUCTION, and this matters more than it looks. Past the + // session probe, `cluster doctor` loads the real kubeconfig and calls the real + // doctor.Run, whose checkBackendEgress probes backendHost("") — i.e. it issues + // a live GET to https://api.tracebloc.io/. On a developer machine with a real + // k3d cluster that is a genuine production request from a unit test. Failing + // loadClusterFn returns right after the session probe, which is everything + // this test needs: the env is decided before it. + origLoad := loadClusterFn + loadClusterFn = func(cluster.KubeconfigOptions) (*cluster.ResolvedConfig, error) { + return nil, errors.New("no cluster (stubbed: keeps this test off the network)") + } + t.Cleanup(func() { loadClusterFn = origLoad }) + + // doctor exits non-zero here (stubbed no-cluster); the assertion is on the env + // it built the client for, which is decided before that. + _, _ = runCmd(t, "cluster", "doctor") + + if gotEnv != api.EnvDev { + t.Fatalf("cluster doctor built its API client for %q, want %q — it must resolve "+ + "through sessionEnv like authedClient, not read cfg.CurrentEnv raw "+ + "(api.BaseURL(%q) is the PROD default, so this probes the wrong backend)", + gotEnv, api.EnvDev, gotEnv) + } +} + +// TestAuthCheckComparesTheResolvedEnv: `auth status --check` compared the raw +// cfg.CurrentEnv against api.ResolveEnv's already-normalised target, so a config +// carrying a non-normalised env failed the probe for the very session it is +// signed in to — and the installer, whose contract is this exit code, would then +// re-run `login` (or skip provisioning) against a session that works. +func TestAuthCheckComparesTheResolvedEnv(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + _, _ = w.Write([]byte(`{"email":"ds@co","account":"Acme"}`)) + } + }) + // withTestBackend isolates the config dir; write a non-normalised env into it. + if err := (&config.Config{CurrentEnv: "Dev", Profiles: map[string]*config.Profile{ + "Dev": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + + if _, err := runCmd(t, "auth", "status", "--check", "--env", "dev"); err != nil { + t.Fatalf("--check --env dev must accept a session stored as \"Dev\" "+ + "(sessionEnv resolves it to dev and the client talks to dev-api), got: %v", err) + } + if !probed { + t.Error("the backend was never probed — the env comparison rejected a session " + + "that differs from the target only in case") + } +} + +// TestAuthCheckStillRejectsARealEnvMismatch is the other half: normalising the +// comparison must not make it lenient about the thing it exists to catch. +func TestAuthCheckStillRejectsARealEnvMismatch(t *testing.T) { + probed := false + withTestBackend(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/userinfo/" { + probed = true + } + }) + saveSignedIn(t, "tok") // CurrentEnv=dev + if _, err := runCmd(t, "auth", "status", "--check", "--env", "stg"); ExitCodeFromError(err) != 1 { + t.Fatalf("exit code = %d, want 1 — a dev session must not satisfy a stg target", + ExitCodeFromError(err)) + } + if probed { + t.Error("must not probe the backend on a genuine env mismatch") + } +} + +// TestAuthStatusShowsTheEnvTheClientWillUse: `auth status` printed the raw +// stored env as its "backend" field, so the human-facing answer to "which +// backend am I on?" could differ from the one --check computes and the one +// authedClient dials — two answers to the same question, in the same file. +func TestAuthStatusShowsTheEnvTheClientWillUse(t *testing.T) { + t.Setenv("TRACEBLOC_CONFIG_DIR", t.TempDir()) + t.Setenv("CLIENT_ENV", "") + if err := (&config.Config{CurrentEnv: " Dev ", Profiles: map[string]*config.Profile{ + " Dev ": {Token: "tok", Email: "ds@co"}, + }}).Save(); err != nil { + t.Fatal(err) + } + out, err := runCmd(t, "auth", "status") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, api.EnvDev) || strings.Contains(out, " Dev ") { + t.Fatalf("auth status reported the stored env verbatim, not the resolved one.\n"+ + "want the %q the client actually dials, got:\n%s", api.EnvDev, out) + } +} + +// TestTheRecordIsLabelledWithTheEnvItWasHanded is the #540 finding. +// +// RecordCommandOutcome resolves the env once and derives BOTH the record's label +// and the sink (spool path + POST destination) from that one value. +// recordCommandOutcome used to call telemetryEnv(signedInEnv()) again for the +// emitter, so the label came from a second, independent config read: two reads +// that merely tend to agree, and disagree the moment a `login` lands between them +// — which is precisely the "labelled stg, posted to prod" leak the comment above +// RecordCommandOutcome claims to prevent. +// +// The invariant is structural, because a race between two config reads is not a +// deterministic test: recordCommandOutcome must label the record with the env it +// was GIVEN. Handing it an env that disagrees with the config on disk is how we +// tell "used the parameter" from "read the config again". +func TestTheRecordIsLabelledWithTheEnvItWasHanded(t *testing.T) { + dir := t.TempDir() + t.Setenv("TRACEBLOC_CONFIG_DIR", dir) + t.Setenv("CLIENT_ENV", "") + // On disk: dev. A second resolution inside recordCommandOutcome would find + // this and label the record "dev". + body := `{"version":2,"current_env":"dev","profiles":{"dev":{"token":"x"}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + if got := signedInEnv(); got != api.EnvDev { + t.Fatalf("signedInEnv() = %q, want %q — the fixture no longer matches the "+ + "on-disk config layout, so this test would pass vacuously", got, api.EnvDev) + } + + var res map[string]string + sink := telemetry.Sink(func(r map[string]string, _ map[string]any) { res = r }) + + root := NewRootCmd(testBuildInfo()) + // Handed: stg — standing in for "the value the sink was built from", which is + // what the caller resolved before the config changed under it. + if err := recordCommandOutcome( + root, root, testBuildInfo(), 0, time.Second, + func(string) string { return "" }, api.EnvStg, sink, + ); err != nil { + t.Fatal(err) + } + if res == nil { + t.Fatal("nothing was delivered") + } + if got := res["deployment.environment"]; got != api.EnvStg { + t.Fatalf("deployment.environment = %q, want %q — the emitter must be labelled "+ + "with the env it was handed (the one the sink was built from), not with a "+ + "second read of the config", got, api.EnvStg) + } +} + +// --- 3. the guard: no NEW resolution site can appear unnoticed ---------------- + +// resolutionSites is the closed set of files in this MODULE allowed to answer +// "which backend?" from ambient state (the config file or $CLIENT_ENV). +// Everything else must take a resolved env as an argument. +// +// This is the rule the last three recuts each re-litigated one site at a time. It +// is here rather than in a reviewer's head because the failure mode is additive: +// every new site looks locally correct, and only the SECOND one is a bug. +// +// Keys are repo-relative because the walk is repo-wide. It used to walk only +// internal/cli, which left the guard blind in 16 of the 17 packages under +// internal/ — i.e. blind exactly where a new site is most likely to land, in a +// package written by someone who never reads internal/cli (Lukas on #551). +var resolutionSites = map[string]string{ + // The primitive: ResolveEnv is the --env/$CLIENT_ENV/prod chain, and the only + // os.Getenv("CLIENT_ENV") in the module. + "internal/api/client.go": "api.ResolveEnv — the primitive chain, and the only $CLIENT_ENV read", + // The --env FLAG, a different question: the env the human/installer NAMED, + // which login persists (the one cfg.CurrentEnv WRITE) and `auth status --check` + // validates against the session. + "internal/cli/auth.go": "api.ResolveEnv(envFlag) — the explicit --env flag, validated by IsKnownEnv", + // sessionEnv: config current_env, else $CLIENT_ENV, else prod. The one chain + // every session-env consumer in internal/cli resolves through. + "internal/cli/client.go": "sessionEnv — the single config -> session-env resolution", + // Storage. Profiles are keyed by the RAW stored string, so this layer must not + // normalise; it hands the raw value out and sessionEnv normalises it. + "internal/config/config.go": "the on-disk current_env field, its accessors, and the v1 migration", + // The CLUSTER's CLIENT_ENV, read off the jobs-manager Deployment — a + // deliberately different question from this CLI's session env. + "internal/doctor/doctor.go": "the cluster's own CLIENT_ENV, for the egress probe's target host", + // internal/cli/telemetry.go is deliberately ABSENT, and the staleness check + // below is what keeps it that way: telemetryEnv/signedInEnv delegate the whole + // chain to sessionEnv and name no needle, so an entry for it would be inert — + // a licence to re-admit a raw read in the very file whose double resolution + // this PR exists to remove (Bugbot on #551). +} + +// envNeedles are the ways a file reads ambient state to answer "which backend?". +// api.BaseURL/IsKnownEnv are pure mappings over an argument and are deliberately +// absent — they resolve nothing. +// +// Deliberately BROAD (bare identifiers, and CLIENT_ENV unquoted so it matches +// help text too): a false positive is a loud line in a diff, a false negative is +// the bug this guard exists to catch. Fail closed. +var envNeedles = []string{"CurrentEnv", "ResolveEnv", "CLIENT_ENV"} + +// matchesAnyNeedle is THE matcher, called from both directions — the detection +// sweep and the allowlist audit. One function on purpose: two copies of "does +// this file resolve an env?" is the same shape as the two copies of "which env?" +// that this PR exists to remove, and it would let detection and allowlisting +// drift apart exactly where nobody looks. +func matchesAnyNeedle(code string) (string, bool) { + for _, needle := range envNeedles { + if strings.Contains(code, needle) { + return needle, true + } + } + return "", false +} + +// envCodeOf tokenises one file, or reports why it could not. +func envCodeOf(path string) (string, error) { + src, err := os.ReadFile(path) + if err != nil { + return "", err + } + return goCodeTokens(string(src)) +} + +func TestNoNewEnvironmentResolutionSiteAppears(t *testing.T) { + const root = "../.." // this test's package dir -> the module root + + // --- detection: what actually resolves an env, module-wide --------------- + matched := map[string]string{} // repo-relative path -> the needle that hit + walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", ".claude", ".worktrees", "vendor", "node_modules", "testdata": + return fs.SkipDir + } + return nil + } + name := d.Name() + if !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + return nil + } + code, err := envCodeOf(path) + if err != nil { + // "cannot tell" is not "clean": abort rather than read a file whose + // tokenisation failed as a string that matches nothing. + return fmt.Errorf("%s: %w", path, err) + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + if needle, ok := matchesAnyNeedle(code); ok { + matched[filepath.ToSlash(rel)] = needle + } + return nil + }) + if walkErr != nil { + t.Fatalf("walking the module: %v", walkErr) + } + + // --- every match must be sanctioned -------------------------------------- + for rel, needle := range matched { + if _, ok := resolutionSites[rel]; !ok { + t.Errorf("%s resolves an environment from ambient state (%q), but is not a "+ + "sanctioned resolution site.\n"+ + "Take the resolved env as an ARGUMENT instead — the CLI must answer "+ + "\"which backend?\" once per invocation and thread it. If this really is "+ + "a new resolution point, add it to resolutionSites with the reason and "+ + "say how it cannot disagree with sessionEnv.", rel, needle) + } + } + + // --- and every sanctioned entry must still EARN its place ---------------- + // + // THE ALLOWLIST MUST STAY A RECORD, NOT BECOME A LICENCE. Asking only whether a + // sanctioned file EXISTS is the defect class this whole PR is about: checking + // the form of a thing instead of the property the form exists to guarantee. An + // entry whose file no longer resolves anything is inert — it verifies nothing, + // while silently pre-approving the next ambient read in that file. My own + // consolidation did exactly that to the telemetry.go entry (Bugbot on #551), + // in the one file whose double resolution this PR removes. + // + // Per ENTRY rather than per suite, so the failure names the entry to delete. + // This also subsumes the "needles went stale" backstop: rename a needle and + // every entry goes inert at once, which is loud and specific rather than a + // single global counter hitting zero. + for rel, why := range resolutionSites { + if _, err := os.Stat(filepath.Join(root, rel)); err != nil { + t.Errorf("resolutionSites lists %s, which cannot be read — stale allowlist", rel) + continue + } + if why == "" { + t.Errorf("resolutionSites[%q] is allowlisted with no reason", rel) + } + if _, ok := matched[rel]; !ok { + t.Errorf("%s is allowlisted as a resolution site but resolves nothing any more "+ + "— drop the entry, or the guard silently permits the next ambient read "+ + "here.", rel) + } + } + + // The one thing the per-entry loop cannot see: an EMPTY allowlist makes it + // vacuous, and a needle rename plus an empty allowlist would then pass in + // silence. Anchor both. + if len(resolutionSites) == 0 || len(matched) == 0 { + t.Fatalf("the guard checked nothing: %d sanctioned entries, %d files matched", + len(resolutionSites), len(matched)) + } +} + +// goCodeTokens renders src as its Go TOKENS, with comments dropped — so the +// guard reads code, never prose (these names are the subject of half the +// comments in the package). +// +// SCANS GO AS GO, because the hand-rolled comment stripper this replaces was +// fail-open on string literals, and Lukas demonstrated both holes on #551: +// the "//" inside a "https://…" literal started a comment that ate the rest of +// the line (needle included), and a "/*" inside a literal like "/*.json" +// swallowed every needle below it to the next "*/" or EOF. Seven non-test files +// in internal/cli already carry an https:// literal, so that was one future line +// away, not a contrived shape. +// +// Literals are KEPT in the output rather than dropped: a needle inside a string +// then reads as a loud false positive, which is cheap — the opposite direction +// fails silently, which is the bug. +// +// SCOPE OF THE ERROR, stated precisely because the first version of this comment +// overclaimed it: go/scanner is LEXICAL, not syntactic, so this returns an error +// only on lexical faults — an unterminated string literal or an unterminated +// /* comment. `func f( {` scans clean and is reported as normal code. That is the +// right guarantee rather than a weak one: the faults it does catch are exactly +// the ones that would desynchronise literal/comment boundaries and hand the +// needles a misread file, which is the failure this function exists to prevent. +// A file that tokenises correctly but does not compile still yields correct +// needles, and `go build` is the check for whether it compiles. +func goCodeTokens(src string) (string, error) { + var fset token.FileSet + var sc scanner.Scanner + scanErrs := 0 + f := fset.AddFile("", fset.Base(), len(src)) + // mode 0: comment tokens are not emitted at all. + sc.Init(f, []byte(src), func(token.Position, string) { scanErrs++ }, 0) + var b strings.Builder + for { + _, tok, lit := sc.Scan() + if tok == token.EOF { + break + } + if lit != "" { + b.WriteString(lit) + } else { + b.WriteString(tok.String()) + } + b.WriteByte(' ') + } + if scanErrs > 0 { + return "", fmt.Errorf("%d scan error(s) — cannot tell what this file reads", scanErrs) + } + return b.String(), nil +} diff --git a/internal/cli/telemetry.go b/internal/cli/telemetry.go index 421b4cbd..f76f7566 100644 --- a/internal/cli/telemetry.go +++ b/internal/cli/telemetry.go @@ -96,10 +96,11 @@ func commandPathOf(c *cobra.Command) string { // resolved exactly the way api.BaseURL resolves it — because that is the host // these records are about. The mapping mirrors BaseURL: a known env is itself; a // present-but-unrecognised value is prod, because api.BaseURL routes every -// unknown value to https://api.tracebloc.io (sessionEnv hands cfg.CurrentEnv to -// api.New verbatim). So prod is the accurate label for that population, not a -// guess — and NOT withheld: a misconfigured install that hits prod and fails is -// exactly the run this feature exists to see. +// unknown value to https://api.tracebloc.io (sessionEnv normalises cfg.CurrentEnv +// but does not validate it, so an unrecognised value reaches api.New intact). So +// prod is the accurate label for that population, not a guess — and NOT withheld: +// a misconfigured install that hits prod and fails is exactly the run this +// feature exists to see. // // $CLIENT_ENV is consulted only when there is no signed-in env, matching // sessionEnv: once cfg.CurrentEnv is set the client ignores $CLIENT_ENV, so @@ -108,13 +109,23 @@ func commandPathOf(c *cobra.Command) string { // // NOTE: that api.BaseURL silently routes an unknown env to prod — so an install // believing it is on another backend sends its token there — is a real defect, -// but in client.go, not here; tracked separately. This function must match that -// behaviour until it changes, not diverge from it. +// but in internal/api/client.go, not here. It is shared with the installer's +// `_backend_url` and contradicted by client-runtime's controller.py (which +// refuses), so it is a three-component decision tracked on backend#2171. This +// function must match that behaviour until it changes, not diverge from it. func telemetryEnv(env string) string { resolved := env if resolved == "" { - // Not signed in: $CLIENT_ENV, then the prod default (as sessionEnv does). - resolved = api.ResolveEnv("") + // No stored session: $CLIENT_ENV, then the prod default — resolved BY + // sessionEnv over an empty config rather than by a second copy of its + // fallback, so the precedence chain lives in exactly one function. + // + // NOT REACHABLE FROM PRODUCTION any more: the only production caller passes + // signedInEnv(), which since it delegates to sessionEnv never returns "". + // Kept, and not dead, because telemetryEnv is a pure mapping that the tests + // call directly with "" — and because a mapping that panics or mislabels on + // an empty input would be a worse contract than one that resolves it. + resolved = sessionEnv(&config.Config{}) } if api.IsKnownEnv(resolved) { return strings.ToLower(resolved) @@ -124,14 +135,27 @@ func telemetryEnv(env string) string { return api.EnvProd } -// signedInEnv reads the environment the config points at, best-effort. A -// missing or unreadable config is simply "not signed in". +// signedInEnv resolves the environment the config points at, best-effort. +// +// Delegates to sessionEnv — the same function authedClient and logout resolve +// through — so the record's label is derived from the session env by the same +// code that picks the host the CLI talks to, not by a parallel restatement of +// the rule that can drift from it. +// +// ALWAYS RETURNS A RESOLVED ENV, never "". It used to return "" for a missing or +// unreadable config, and the old comment called that "not signed in"; delegating +// to sessionEnv means that case now resolves through $CLIENT_ENV to prod like any +// other empty config. So "not signed in" no longer names an output — it means +// "resolved from $CLIENT_ENV/prod rather than from a stored session", and the two +// are indistinguishable here by design (the label is about the host, not the +// session). Do NOT write `if signedInEnv() == ""` on the strength of a stale +// reading of this: it cannot fire. func signedInEnv() string { cfg, err := config.Load() if err != nil || cfg == nil { - return "" + cfg = &config.Config{} // unreadable == no stored session } - return cfg.CurrentEnv + return sessionEnv(cfg) } // processInstanceID is the per-PROCESS id §2 asks for off-cluster. @@ -158,27 +182,38 @@ func processInstanceID() string { // was unhappy would be a strictly worse CLI. A malformed event is caught by the // tests below, where it is free. func RecordCommandOutcome(root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration) { - // The sink is built from the SAME resolved env the emitter is labelled with, - // not from a second read of the config. Two independent resolutions here is - // how a record ends up labelled `stg` and posted to prod. + // THE ONE RESOLUTION POINT for this invocation. The label, the spool and the + // POST destination are all derived from this single value: two independent + // resolutions is how a record ends up labelled `stg` and posted to prod. + // + // It is resolved here and threaded down as a parameter rather than re-read + // inside recordCommandOutcome, because "both call telemetryEnv(signedInEnv())" + // is not one resolution — it is two config reads that merely tend to agree, + // and a `login` landing between them makes them disagree (Bugbot on #540). env := telemetryEnv(signedInEnv()) - _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, pendingSink(env)) + _ = recordCommandOutcome(root, executed, info, exitCode, elapsed, os.Getenv, env, pendingSink(env)) } -// recordCommandOutcome is RecordCommandOutcome with its two ambient -// dependencies passed in, so the tests drive the real thing. +// recordCommandOutcome is RecordCommandOutcome with its ambient dependencies +// passed in, so the tests drive the real thing. +// +// TAKES THE RESOLVED env, and never resolves one itself — the same rule, and for +// the same reason, as deliver/pendingSink in telemetry_transport.go: the emitter's +// label must be the value the sink was built from, not a second look at the +// config that happens to land on it. func recordCommandOutcome( root, executed *cobra.Command, info BuildInfo, exitCode int, elapsed time.Duration, getenv func(string) string, + env string, sink telemetry.Sink, ) error { if !telemetryEnabled(getenv) { return nil } - emitter := telemetry.New(telemetryEnv(signedInEnv()), info.Version, processInstanceID()) + emitter := telemetry.New(env, info.Version, processInstanceID()) if sink != nil { emitter.SetSink(sink) } diff --git a/internal/cli/telemetry_test.go b/internal/cli/telemetry_test.go index 61de0406..05056775 100644 --- a/internal/cli/telemetry_test.go +++ b/internal/cli/telemetry_test.go @@ -49,8 +49,13 @@ func captureOutcome( delivered++ }) getenv := func(k string) string { return env[k] } + // Resolve the env the way RecordCommandOutcome does and hand the SAME value + // to the recorder, so these tests exercise the production path end to end + // (config -> label) now that recordCommandOutcome no longer resolves for + // itself. TestTheRecordIsLabelledWithTheEnvItWasHanded pins the threading. if err := recordCommandOutcome( - root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, sink, + root, executed, testBuildInfo(), exitCode, 1500*time.Millisecond, getenv, + telemetryEnv(signedInEnv()), sink, ); err != nil { t.Fatalf("recordCommandOutcome: %v", err) } @@ -298,8 +303,8 @@ func TestTheEnvironmentLabelMatchesTheBackend(t *testing.T) { func TestASignedInUnknownEnvIgnoresClientEnv(t *testing.T) { // The bug this pins (Asad, cli#528 review): the client resolves a signed-in - // env via sessionEnv, which returns cfg.CurrentEnv VERBATIM and never consults - // $CLIENT_ENV — so a config on "banana" talks to prod (api.BaseURL default) + // env via sessionEnv, which normalises cfg.CurrentEnv but never falls back to + // $CLIENT_ENV while it is set — so a config on "banana" talks to prod (api.BaseURL default) // regardless of $CLIENT_ENV. The old code resolved the label through // ResolveEnv, which DOES read $CLIENT_ENV, so it filed the run under "dev" // while every request went to prod. The label must be prod, not dev. @@ -356,8 +361,9 @@ func TestTheSignedInEnvironmentWins(t *testing.T) { func TestASignedInUnknownEnvironmentIsLabelledProd(t *testing.T) { // A run signed into an environment the CLI does not recognise talks to prod - // (sessionEnv hands cfg.CurrentEnv to api.New verbatim, api.BaseURL routes the - // unknown value to prod), so its record must be filed under prod — that + // (sessionEnv normalises but does not validate cfg.CurrentEnv, so the unknown + // value reaches api.New and api.BaseURL routes it to prod), so its record must + // be filed under prod — that // failed-install-on-prod run is exactly what this feature exists to capture. dir := t.TempDir() t.Setenv("TRACEBLOC_CONFIG_DIR", dir) diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 83ffa661..3c31b0ea 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -29,6 +29,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + "github.com/tracebloc/cli/internal/api" "github.com/tracebloc/cli/internal/cluster" "github.com/tracebloc/cli/internal/resources" ) @@ -506,18 +507,25 @@ func checkBackendEgress(ctx context.Context, env map[string]string, probe func(c // backendHost maps CLIENT_ENV to the backend API host, mirroring the edge // runtime's own mapping (controller.py). Unset/unknown defaults to prod, the // chart's CLIENT_ENV default. +// +// DERIVED FROM api.BaseURL, not restated. The env→host mapping used to be a +// second copy of BaseURL's switch living in this package, which is how the two +// drift: the same three hosts written down twice, with nothing that fails when +// only one of them is edited. api.BaseURL already lower-cases, so TrimSpace is +// the only normalisation this adds — a CLIENT_ENV read off a container spec can +// carry surrounding whitespace that a --env flag cannot. +// +// The input is the CLUSTER's CLIENT_ENV (read off the jobs-manager Deployment), +// not this CLI's session env — a deliberately different question, which is why +// this takes a string rather than calling into the session resolution. func backendHost(clientEnv string) string { - // Normalize like the API client (api.ResolveEnv/BaseURL lower-case), so a - // non-lowercase CLIENT_ENV on the edge box doesn't fall through to prod and - // make the doctor probe the wrong backend. - switch strings.ToLower(strings.TrimSpace(clientEnv)) { - case "dev": - return "dev-api.tracebloc.io" - case "stg": - return "stg-api.tracebloc.io" - default: + u, err := url.Parse(api.BaseURL(strings.TrimSpace(clientEnv))) + if err != nil || u.Host == "" { + // Unreachable for BaseURL's closed set of return values; the prod default + // keeps this total rather than returning an empty host into a probe URL. return "api.tracebloc.io" } + return u.Host } // checkRequestsProxy verifies the requests-proxy deployment is present and