Merge pull request cli#13205 from cli/wm/record-official-extension-telemetry

Record official extension telemetry

Author: williammartin

acceptance/testdata/telemetry/no-telemetry-for-extension.txtar

@@ -1,4 +1,6 @@
-# Extensions should not generate telemetry events
+# Third-party extensions must not generate telemetry events, since the
+# extension command name can be a user-authored identifier (e.g. an
+# organization or project name).
 [!exec:bash] skip
 
 env GH_PRIVATE_ENABLE_TELEMETRY=1

acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar

@@ -0,0 +1,20 @@
+# Official extension stubs (the hidden commands suggesting installation of
+# GitHub-owned extensions) are safe to report via telemetry: their command
+# names come from a fixed, hard-coded registry and do not contain any
+# user-authored identifiers.
+
+env GH_PRIVATE_ENABLE_TELEMETRY=1
+env GH_TELEMETRY=log
+env GH_TELEMETRY_SAMPLE_RATE=100
+
+# `stack` is registered in extensions.OfficialExtensions. Since no real
+# extension is installed, the hidden stub runs and, in a non-TTY session,
+# prints install instructions without prompting.
+exec gh stack
+stderr 'gh extension install github/gh-stack'
+
+# The stub invocation records a command_invocation event for the stub's
+# command path.
+stderr 'Telemetry payload:'
+stderr '"type": "command_invocation"'
+stderr '"command": "gh stack"'

pkg/cmd/root/extension.go

@@ -79,7 +79,14 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex
 	}
 
 	cmdutil.DisableAuthCheck(cmd)
-	cmdutil.DisableTelemetry(cmd)
+	// Extensions are user-installed and their names can be arbitrary
+	// (potentially including sensitive identifiers such as project or
+	// organization names), so we must not record telemetry for them by
+	// default. Official GitHub-owned extensions are a known, fixed set and
+	// can safely contribute their command name to telemetry.
+	if !extensions.IsOfficial(ext.Name(), ext.Owner()) {
+		cmdutil.DisableTelemetry(cmd)
+	}
 
 	return cmd
 }

pkg/cmd/root/extension_registration_test.go

@@ -57,6 +57,9 @@ func TestNewCmdRoot_ExtensionRegistration(t *testing.T) {
 					NameFunc: func() string {
 						return extName
 					},
+					OwnerFunc: func() string {
+						return ""
+					},
 				})
 			}
 

pkg/cmd/root/extension_test.go

@@ -144,6 +144,9 @@ func TestNewCmdExtension_Updates(t *testing.T) {
 			NameFunc: func() string {
 				return tt.extName
 			},
+			OwnerFunc: func() string {
+				return ""
+			},
 			UpdateAvailableFunc: func() bool {
 				return tt.extUpdateAvailable
 			},
@@ -199,6 +202,9 @@ func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) {
 		NameFunc: func() string {
 			return "major-update"
 		},
+		OwnerFunc: func() string {
+			return ""
+		},
 		UpdateAvailableFunc: func() bool {
 			return true
 		},
@@ -234,3 +240,60 @@ func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) {
 		t.Fatal("extension update check should have exited")
 	}
 }
+
+func TestNewCmdExtension_TelemetryEnabledForOfficialExtensions(t *testing.T) {
+	tests := []struct {
+		name             string
+		extName          string
+		extOwner         string
+		wantTelemetryOff bool
+	}{
+		{
+			name:             "official extension records telemetry",
+			extName:          "stack",
+			extOwner:         "github",
+			wantTelemetryOff: false,
+		},
+		{
+			name:             "official name with third-party owner disables telemetry",
+			extName:          "stack",
+			extOwner:         "williammartin",
+			wantTelemetryOff: true,
+		},
+		{
+			name:             "official name with empty owner disables telemetry",
+			extName:          "stack",
+			extOwner:         "",
+			wantTelemetryOff: true,
+		},
+		{
+			name:             "official extension name with mixed case disables telemetry",
+			extName:          "STACK",
+			extOwner:         "github",
+			wantTelemetryOff: true,
+		},
+		{
+			name:             "third-party extension disables telemetry",
+			extName:          "my-custom-ext",
+			extOwner:         "someone",
+			wantTelemetryOff: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ios, _, _, _ := iostreams.Test()
+			em := &extensions.ExtensionManagerMock{}
+			ext := &extensions.ExtensionMock{
+				NameFunc:  func() string { return tt.extName },
+				OwnerFunc: func() string { return tt.extOwner },
+			}
+
+			cmd := root.NewCmdExtension(ios, em, ext, func(extensions.ExtensionManager, extensions.Extension) (*update.ReleaseInfo, error) {
+				return nil, nil
+			})
+
+			assert.Equal(t, tt.wantTelemetryOff, cmd.Annotations["telemetry"] == "disabled")
+		})
+	}
+}

pkg/extensions/official.go

@@ -1,6 +1,8 @@
 package extensions
 
 import (
+	"strings"
+
 	"github.com/cli/cli/v2/internal/ghrepo"
 )
 
@@ -24,3 +26,28 @@ var OfficialExtensions = []OfficialExtension{
 	{Name: "aw", Owner: "github", Repo: "gh-aw"},
 	{Name: "stack", Owner: "github", Repo: "gh-stack"},
 }
+
+// IsOfficial reports whether the given extension command name and owner
+// match an entry in the OfficialExtensions registry. Owner must be
+// checked alongside name because a user may have installed a third-party
+// extension that happens to share a name with one of ours (e.g.
+// `someuser/gh-stack` predates `github/gh-stack` becoming official).
+// Owner will be empty for local extensions, in which case the extension
+// is treated as non-official.
+//
+// Comparison is case-sensitive: on case-sensitive filesystems a user can
+// install a private extension whose name differs only in casing (e.g.
+// `gh-STACK`), and we must not treat that as official. Owner comparison
+// is case-insensitive because GitHub usernames and organization names
+// are themselves case-insensitive.
+func IsOfficial(name, owner string) bool {
+	if owner == "" {
+		return false
+	}
+	for _, ext := range OfficialExtensions {
+		if ext.Name == name && strings.EqualFold(ext.Owner, owner) {
+			return true
+		}
+	}
+	return false
+}

pkg/extensions/official_test.go

@@ -13,3 +13,61 @@ func TestOfficialExtension_Repository(t *testing.T) {
 	assert.Equal(t, "gh-stack", repo.RepoName())
 	assert.Equal(t, "github.com", repo.RepoHost())
 }
+
+func TestIsOfficial(t *testing.T) {
+	tests := []struct {
+		name     string
+		extName  string
+		extOwner string
+		want     bool
+	}{
+		{
+			name:     "known official extension matches",
+			extName:  "stack",
+			extOwner: "github",
+			want:     true,
+		},
+		{
+			name:     "official name with different owner is not official",
+			extName:  "stack",
+			extOwner: "williammartin",
+			want:     false,
+		},
+		{
+			name:     "official name with empty owner is not official",
+			extName:  "stack",
+			extOwner: "",
+			want:     false,
+		},
+		{
+			name:     "owner comparison is case-insensitive",
+			extName:  "stack",
+			extOwner: "GitHub",
+			want:     true,
+		},
+		{
+			name:     "mixed-case name does not match",
+			extName:  "STACK",
+			extOwner: "github",
+			want:     false,
+		},
+		{
+			name:     "unknown name is not official",
+			extName:  "not-a-real-extension",
+			extOwner: "github",
+			want:     false,
+		},
+		{
+			name:     "empty name is not official",
+			extName:  "",
+			extOwner: "github",
+			want:     false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, IsOfficial(tt.extName, tt.extOwner))
+		})
+	}
+}