Skip to content

Commit f69a20c

Browse files
committed
fix(telemetry): redact aliased flags, gate rebind, fix categorization
Addresses code-review findings: 1. redact.go (HIGH): `createos login -t <token>` leaked the raw token because LocalFlagNames returned the alias "t" and RedactFlagValue's substring match did not catch it. FlagsFromContext now resolves the cli.Flag via Lineage and redacts when ANY of its Names() (canonical or alias) matches the denylist. 2. login.go (MEDIUM): If GetUser or SaveIdentity failed, RebindIdentity could load a stale .identity from a previous user and mis-attribute login telemetry. Now we only rebind when SaveIdentity succeeds for the current account; otherwise delete any stale identity file and skip rebind. 3. errors.go (LOW): "non-interactive mode: use --token flag to sign in" was bucketed as auth because the auth needles ran first. Re-ordered: user_input substrings now check first. 4. README.md (LOW): Telemetry disclosure no longer claims data is anonymous unconditionally; clarifies that events are anonymous pre-login and tied to account_id (and project_id when applicable) post-login.
1 parent 46862f0 commit f69a20c

4 files changed

Lines changed: 92 additions & 11 deletions

File tree

README.md

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,9 @@ createos environments list --project <id> -o json
470470

471471
## Telemetry
472472

473-
The CLI sends anonymous usage data (commands run, version, OS, error categories)
474-
to help us improve the product. No file paths, command output, or secrets are
473+
The CLI sends usage telemetry (commands run, version, OS, error categories)
474+
to help us improve the product. Before you run `createos login`, events are
475+
anonymous and tied only to a one-way machine hash. After login, events are
476+
associated with your account ID and may include the project ID for
477+
project-scoped commands. No file paths, command output, or secrets are
475478
collected. To disable, set `CREATEOS_DO_NOT_TRACK=1` in your environment.

cmd/auth/login.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,20 +43,35 @@ func captureLoginFailure(method string, err error) {
4343
// fetch /me, persist Identity (preserving AliasedForUserID for same user),
4444
// rebind telemetry distinct_id to user_id, then emit success auth_event.
4545
// All identity fetching is best-effort — a failure here must NOT fail login.
46+
//
47+
// If /me fails OR SaveIdentity fails, we DELETE any pre-existing .identity
48+
// file and skip RebindIdentity — otherwise stale identity from a previous
49+
// account on this machine would mis-attribute the new login's telemetry.
4650
func bindIdentityAndCapture(apiClient *api.APIClient, method string) {
51+
identityFresh := false
4752
if apiClient != nil {
4853
if u, err := apiClient.GetUser(); err == nil && u != nil && u.ID != "" {
4954
id := config.Identity{UserID: u.ID}
5055
if existing, _ := config.LoadIdentity(); existing != nil && existing.UserID == u.ID {
5156
id.AliasedForUserID = existing.AliasedForUserID
5257
}
53-
_ = config.SaveIdentity(id)
58+
if saveErr := config.SaveIdentity(id); saveErr == nil {
59+
identityFresh = true
60+
}
5461
}
5562
// Silent on /me failure — login still succeeds without user_id.
5663
}
5764

65+
if !identityFresh {
66+
// /me or SaveIdentity failed; drop any stale identity so that a later
67+
// command does not attribute events to a previous user_id.
68+
_ = config.DeleteIdentity()
69+
}
70+
5871
if telemetry.Default != nil {
59-
telemetry.Default.RebindIdentity()
72+
if identityFresh {
73+
telemetry.Default.RebindIdentity()
74+
}
6075
telemetry.Default.Capture("auth_event", map[string]any{
6176
"action": "login",
6277
"method": method,

internal/telemetry/errors.go

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
// Order of checks (first match wins):
1010
// 1. *api.APIError → category by HTTP status.
1111
// 2. context.DeadlineExceeded / net.Error → "network".
12-
// 3. local sentinel substrings → "auth" or "user_input".
12+
// 3. local sentinel substrings → "user_input" first, then "auth".
13+
// user_input is checked first because some validation messages contain
14+
// auth-shaped phrases (e.g. "use --token flag to sign in").
1315
// 4. default → "unknown".
1416
package telemetry
1517

@@ -57,15 +59,17 @@ func CategorizeError(err error) (category string, apiStatusCode int) {
5759
}
5860

5961
// 3. Locally-raised sentinels — string match on err.Error().
62+
// user_input first: some validation messages embed "sign in" wording
63+
// (e.g. "non-interactive mode: use --token flag to sign in").
6064
msg := err.Error()
61-
for _, needle := range authNeedles {
65+
for _, needle := range userInputNeedles {
6266
if strings.Contains(msg, needle) {
63-
return "auth", 0
67+
return "user_input", 0
6468
}
6569
}
66-
for _, needle := range userInputNeedles {
70+
for _, needle := range authNeedles {
6771
if strings.Contains(msg, needle) {
68-
return "user_input", 0
72+
return "auth", 0
6973
}
7074
}
7175

internal/telemetry/redact.go

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,65 @@ const redactedSentinel = "[REDACTED]"
1919
// RedactFlagValue returns the original value or the sentinel when the flag
2020
// name matches any deny keyword (case-insensitive substring match).
2121
func RedactFlagValue(name string, value any) any {
22+
if isSensitiveName(name) {
23+
return redactedSentinel
24+
}
25+
return value
26+
}
27+
28+
// isSensitiveName reports whether a flag name (canonical or alias) matches
29+
// any deny keyword via case-insensitive substring match.
30+
func isSensitiveName(name string) bool {
2231
lower := strings.ToLower(name)
2332
for _, kw := range denyKeywords {
2433
if strings.Contains(lower, kw) {
25-
return redactedSentinel
34+
return true
2635
}
2736
}
28-
return value
37+
return false
38+
}
39+
40+
// findFlagByName walks c.Lineage() (child→root) and returns the cli.Flag whose
41+
// Names() include name (canonical or alias). Returns nil when not found.
42+
func findFlagByName(c *cli.Context, name string) cli.Flag {
43+
if c == nil {
44+
return nil
45+
}
46+
for _, ctx := range c.Lineage() {
47+
if ctx.Command != nil {
48+
for _, f := range ctx.Command.Flags {
49+
for _, n := range f.Names() {
50+
if n == name {
51+
return f
52+
}
53+
}
54+
}
55+
}
56+
if ctx.App != nil {
57+
for _, f := range ctx.App.Flags {
58+
for _, n := range f.Names() {
59+
if n == name {
60+
return f
61+
}
62+
}
63+
}
64+
}
65+
}
66+
return nil
67+
}
68+
69+
// anyAliasSensitive returns true when any of the flag's Names()
70+
// (canonical + aliases) matches the denylist.
71+
func anyAliasSensitive(f cli.Flag) bool {
72+
if f == nil {
73+
return false
74+
}
75+
for _, n := range f.Names() {
76+
if isSensitiveName(n) {
77+
return true
78+
}
79+
}
80+
return false
2981
}
3082

3183
// NormalizeAPIURL strips path/query/fragment, returning "scheme://host" only.
@@ -60,6 +112,13 @@ func FlagsFromContext(c *cli.Context) map[string]any {
60112
continue
61113
}
62114
}
115+
// Canonicalize: redact when ANY alias of this flag matches the
116+
// denylist, not just the user-supplied alias. Example: `login -t <T>`
117+
// reports name="t" via LocalFlagNames; the canonical "token" matches.
118+
if anyAliasSensitive(findFlagByName(c, name)) {
119+
out[name] = redactedSentinel
120+
continue
121+
}
63122
out[name] = RedactFlagValue(name, v)
64123
}
65124
return out

0 commit comments

Comments
 (0)